Bug 128031 – Simplified name scope creation for function expressions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

Simplified name scope creation for function expressions

[Geoffrey Garen]

Created attachment 222866 [details]
Patch

[WebKit Commit Bot]

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.

[Mark Lam]

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.

[Geoffrey Garen]

> > 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.

[Mark Lam]

(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.

[Geoffrey Garen]

Committed r163321: <http://trac.webkit.org/changeset/163321>