[webkit-reviews] review granted: [Bug 190340] [JSC] JSC should have "parseFunction" to optimize Function constructor : [Attachment 351774] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Oct 11 13:46:51 PDT 2018


Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki
<yusukesuzuki at slowstart.org>'s request for review:
Bug 190340: [JSC] JSC should have "parseFunction" to optimize Function
constructor
https://bugs.webkit.org/show_bug.cgi?id=190340

Attachment 351774: Patch

https://bugs.webkit.org/attachment.cgi?id=351774&action=review




--- Comment #25 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 351774
  --> https://bugs.webkit.org/attachment.cgi?id=351774
Patch

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

r=me

> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp:177
> +    JSObject*& exception, int overrideLineNumber, std::optional<int>
parametersEndPosition)

Since parametersEndPosition is only ever used for FunctionConstructors, can you
rename it as functionConstructorParametersEndPosition everywhere below?  I
think that that documents its intent better.

> Source/JavaScriptCore/parser/Parser.cpp:247
> +	   else {
> +	       if (parsingContext == ParsingContext::FunctionConstructor)
> +		   sourceElements = parseSingleFunction(context,
parametersEndPosition);
> +	       else
> +		   sourceElements = parseSourceElements(context,
CheckForStrictMode);
> +	   }

No need to indent this.  You can just express this as:

    else if (parsingContext == ParsingContext::FunctionConstructor)
	sourceElements = parseSingleFunction(context, parametersEndPosition);
    else
	sourceElements = parseSourceElements(context, CheckForStrictMode);

>> Source/JavaScriptCore/parser/Parser.cpp:2502
>> +		semanticFailIfFalse(lastTokenEndPosition().offset ==
*parametersEndPosition, "Parameters should be composed of arguments offered for
parameters in Function constructor");
> 
> I'm not sure if I'm right here because of some edge cases, but maybe the
message "Parameters should match arguments offered as parameters in Function
constructor" is more clear about the error.

I like Caio's suggestion "Parameters should match arguments offered as
parameters in Function constructor" because it's more concise.

>> Source/JavaScriptCore/parser/Parser.cpp:2935
>> +		failIfFalse((parseFunctionInfo(context,
FunctionNameRequirements::Unnamed, parseMode, false, isConstructor ?
constructorKind : ConstructorKind::None, SuperBinding::Needed, methodStart,
methodInfo, FunctionDefinitionType::Method, std::nullopt)), "Cannot parse this
method");
> 
> Maybe making ```parametersEndPosition``` default to ```std::nullopt``` in
```parseFunctionInfo``` would make this code more readable.

I can go either way with this.	There are already places in the code where
you're relying on having a default value for the parametersEndPosition.  For
that reason, I'm slightly more include to prefer using a default value for this
case as well.  I'll leave this to your discretion.


More information about the webkit-reviews mailing list