Closed
Bug 669369
Opened 13 years ago
Closed 13 years ago
Try simplifying Parser::setFunctionKinds and removing funarg analysis
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: jorendorff, Assigned: jorendorff)
Details
Attachments
(1 file)
12.17 KB,
patch
|
dmandelin
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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)
Updated•13 years ago
|
Attachment #553986 -
Flags: review?(dmandelin) → review+
Assignee | ||
Comment 2•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/77c5e531f7cb
Whiteboard: [inbound]
Comment 3•13 years ago
|
||
Comment on attachment 553986 [details] [diff] [review] v1 >+ uintN hasUpvars = false; Why is this an int?
Comment 4•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/77c5e531f7cb
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Assignee | ||
Comment 5•13 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
Comment 6•13 years ago
|
||
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.
Description
•