The default bug view has changed. See this FAQ.

Try simplifying Parser::setFunctionKinds and removing funarg analysis

RESOLVED FIXED in mozilla9

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Trunk
mozilla9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 553986 [details] [diff] [review]
v1

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+
(Assignee)

Comment 2

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/77c5e531f7cb
Whiteboard: [inbound]
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
(Assignee)

Comment 5

6 years ago
(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.