Closed Bug 669369 Opened 11 years ago Closed 11 years ago

Try simplifying Parser::setFunctionKinds and removing funarg analysis


(Core :: JavaScript Engine, defect)

Not set





(Reporter: jorendorff, Assigned: jorendorff)



(1 file)

In bug 592202 comment 52 I wrote:
> I'd like another follow-up bug to look at the `if (!fn->isFunArg())` block
> in Parser::setFunctionKinds. I bet those two blocks of code (the if block and
> the else block) can be combined into something shorter and saner.

Brendan replied in comment 55:
> As mentioned on IRC, that condition is a stricter one than the condition we
> need now: that the function isn't induced by a hoisted declaration form. We
> cannot flatten non-lambdas.

...which is not total disagreement, so I think I will try it.

There's also this, in CanFlattenUpvar:
>    /*
>     * If this function is reaching up across an enclosing funarg, then we
>     * cannot copy dn's value into a flat closure slot (the display stops
>     * working once the funarg escapes).
>     */
>    if (!afunbox || afunbox->node->isFunArg())
>        return false;

This looks bogus to me; I think it can be deleted.

If setFunctionKinds and CanFlattenUpvar can be simplified this way, then the funarg analysis can be removed.
Attached patch v1Splinter Review
This patch makes me happy.

As long as the implementation of js::GetUpvar is to walk the stack, we can't get rid of the funarg analysis.
Assignee: general → jorendorff
Attachment #553986 - Flags: review?(dmandelin)
Attachment #553986 - Flags: review?(dmandelin) → review+
Comment on attachment 553986 [details] [diff] [review]

>+            uintN hasUpvars = false;

Why is this an int?
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
(In reply to Ms2ger from comment #3)
> Comment on attachment 553986 [details] [diff] [review]
> >+            uintN hasUpvars = false;
> Why is this an int?

Oh, it shouldn't be. The obvious fix is now inbound:
Whiteboard: [inbound]
Version: Other Branch → Trunk
You need to log in before you can comment on or make changes to this bug.