Last Comment Bug 669369 - Try simplifying Parser::setFunctionKinds and removing funarg analysis
: Try simplifying Parser::setFunctionKinds and removing funarg analysis
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla9
Assigned To: Jason Orendorff [:jorendorff]
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2011-07-05 10:41 PDT by Jason Orendorff [:jorendorff]
Modified: 2011-09-01 01:39 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v1 (12.17 KB, patch)
2011-08-17 19:53 PDT, Jason Orendorff [:jorendorff]
dmandelin: review+
Details | Diff | Splinter Review

Description User image Jason Orendorff [:jorendorff] 2011-07-05 10:41:53 PDT
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.
Comment 1 User image Jason Orendorff [:jorendorff] 2011-08-17 19:53:17 PDT
Created attachment 553986 [details] [diff] [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.
Comment 2 User image Jason Orendorff [:jorendorff] 2011-08-30 17:39:20 PDT
Comment 3 User image :Ms2ger (⌚ UTC+1/+2) 2011-08-31 02:01:30 PDT
Comment on attachment 553986 [details] [diff] [review]

>+            uintN hasUpvars = false;

Why is this an int?
Comment 4 User image Marco Bonardo [::mak] 2011-08-31 02:07:55 PDT
Comment 5 User image Jason Orendorff [:jorendorff] 2011-08-31 08:43:42 PDT
(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:
Comment 6 User image Ed Morley [:emorley] 2011-09-01 01:39:39 PDT

Note You need to log in before you can comment on or make changes to this bug.