Closed Bug 669369 Opened 9 years ago Closed 9 years ago

Try simplifying Parser::setFunctionKinds and removing funarg analysis

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

Attachments

(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]
v1

>+            uintN hasUpvars = false;

Why is this an int?
http://hg.mozilla.org/mozilla-central/rev/77c5e531f7cb
Status: NEW → RESOLVED
Closed: 9 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:
  http://hg.mozilla.org/integration/mozilla-inbound/rev/0504a441fef7
http://hg.mozilla.org/mozilla-central/rev/0504a441fef7
Whiteboard: [inbound]
Version: Other Branch → Trunk
You need to log in before you can comment on or make changes to this bug.