Bug 128031

Summary: Simplified name scope creation for function expressions
Product: WebKit Reporter: Geoffrey Garen <ggaren>
Component: New BugsAssignee: Geoffrey Garen <ggaren>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, mark.lam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch mark.lam: review+

Description Geoffrey Garen 2014-01-31 17:12:56 PST
Simplified name scope creation for function expressions
Comment 1 Geoffrey Garen 2014-01-31 17:29:40 PST
Created attachment 222866 [details]
Patch
Comment 2 WebKit Commit Bot 2014-01-31 17:32:21 PST
Attachment 222866 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSNameScope.h:77:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Mark Lam 2014-01-31 19:10:30 PST
Comment on attachment 222866 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=222866&action=review

Other than the part in isNumericCompareFunction() which I'm unsure about, everything else looks good to me.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:86
> +    JSScope* scope = callData.js.scope;
>  
> -    JSObject* error = executable->prepareForExecution(exec, callData.js.scope, CodeForCall);
> +    JSObject* error = executable->prepareForExecution(exec, jsCast<JSFunction*>(function), &scope, CodeForCall);

Is there a reason you didn't want prepareForExecution() to update the scope in callData.js.scope?  I'm not sure which (to update or not to update) is the right thing to do yet, but thought I'd bring this to your attention and ask in case this was not intentional.
Comment 4 Geoffrey Garen 2014-01-31 19:41:16 PST
> > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:86
> > +    JSScope* scope = callData.js.scope;
> >  
> > -    JSObject* error = executable->prepareForExecution(exec, callData.js.scope, CodeForCall);
> > +    JSObject* error = executable->prepareForExecution(exec, jsCast<JSFunction*>(function), &scope, CodeForCall);
> 
> Is there a reason you didn't want prepareForExecution() to update the scope in callData.js.scope?  I'm not sure which (to update or not to update) is the right thing to do yet, but thought I'd bring this to your attention and ask in case this was not intentional.

callData.js.scope is const, so we can't update it. It's const so a class can return the same piece of data each time, if it likes.

In this case, we don't need to update our scope because we don't use it. We're only compiling in order to analyze the code -- we're not running it.
Comment 5 Mark Lam 2014-01-31 22:06:08 PST
(In reply to comment #4)
> callData.js.scope is const, so we can't update it. It's const so a class can return the same piece of data each time, if it likes.

I should have noticed the const.

> In this case, we don't need to update our scope because we don't use it. We're only compiling in order to analyze the code -- we're not running it.

Sounds good.  r=me.
Comment 6 Geoffrey Garen 2014-02-03 12:34:47 PST
Committed r163321: <http://trac.webkit.org/changeset/163321>