[webkit-reviews] review granted: [Bug 175210] [ESNext] Async iteration - Implement Async Generator - parser : [Attachment 317298] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <utatane.tea at gmail.com> has granted GSkachkov
<gskachkov at gmail.com>'s request for review:
Bug 175210: [ESNext] Async iteration - Implement Async Generator - parser
https://bugs.webkit.org/show_bug.cgi?id=175210

Attachment 317298: Patch

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




--- Comment #2 from Yusuke Suzuki <utatane.tea at gmail.com> ---
Comment on attachment 317298
  --> https://bugs.webkit.org/attachment.cgi?id=317298
Patch

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

r=+ with nits.

> Source/JavaScriptCore/parser/ASTBuilder.h:440
> +	   SourceParseMode sourceParseMode;

Let's rename this to `bodySourceParseMode` and set `bodySourceParseMode =
mode;` first.

> Source/JavaScriptCore/parser/ASTBuilder.h:451
> +	   } else
> +	       sourceParseMode = mode;

Drop this.

> Source/JavaScriptCore/parser/Parser.cpp:2866
> +	       if (isAsyncMethodParseMode(parseMode) ||
isAsyncGeneratorMethodParseMode(parseMode)) {
>		   isConstructor = false;
> -		   parseMode = SourceParseMode::AsyncMethodMode;
> -		   semanticFailIfTrue(*ident == m_vm->propertyNames->prototype,
"Cannot declare an async method named 'prototype'");
> -		   semanticFailIfTrue(*ident ==
m_vm->propertyNames->constructor, "Cannot declare an async method named
'constructor'");
> -	       } else if (isGenerator) {
> +		   semanticFailIfTrue(*ident == m_vm->propertyNames->prototype,
"Cannot declare ", stringArticleForFunctionMode(parseMode),
stringForFunctionMode(parseMode), " named 'prototype'");
> +		   semanticFailIfTrue(*ident ==
m_vm->propertyNames->constructor, "Cannot declare ",
stringArticleForFunctionMode(parseMode), stringForFunctionMode(parseMode), "
named 'constructor'");
> +	       } else if (isGeneratorMethodParseMode(parseMode)) {
>		   isConstructor = false;
>		   parseMode = SourceParseMode::GeneratorWrapperMethodMode;
>		   semanticFailIfTrue(*ident == m_vm->propertyNames->prototype,
"Cannot declare a generator named 'prototype'");
>		   semanticFailIfTrue(*ident ==
m_vm->propertyNames->constructor, "Cannot declare a generator named
'constructor'");
>	       }

I think this `if (isGeneratorMethodParseMode(parseMode))` case can be merged
into the above async case if you add a generator case to
stringArticleForFunctionMode and stringForFunctionMode.
And we do not need to execute `parseMode =
SourceParseMode::GeneratorWrapperMethodMode;` since parseMode is already
SourceParseMode::GeneratorWrapperMethodMode.

> Source/JavaScriptCore/parser/Parser.cpp:3858
> +    bool isIdentSet = false;

I think this is always `false`, right? Nobody changes this isIdentSet.
What is the purpose of this variable?

> Source/JavaScriptCore/parser/Parser.cpp:3861
> +    const Identifier* ident;
> +    unsigned getterOrSetterStartOffset;
> +    JSToken identToken;

These changes are not necessary. Let's revert this.

> Source/JavaScriptCore/parser/Parser.cpp:3896
> -	   JSToken identToken = m_token;
> -	   const Identifier* ident = m_token.m_data.ident;
> -	   unsigned getterOrSetterStartOffset = tokenStart();
> +	   identToken = m_token;
> +	   ident = m_token.m_data.ident;
> +	   getterOrSetterStartOffset = tokenStart();

I don't think these changes are necessary. Let's revert this.

> Source/JavaScriptCore/parser/Parser.cpp:3900
> +	   else if (UNLIKELY(!isIdentSet))

This is always `false`, right?

> Source/JavaScriptCore/parser/Parser.h:1514
> +    ALWAYS_INLINE SourceParseMode
getAsynFunctionBodyParseMode(SourceParseMode parseMode)
> +    {
> +	   if (isAsyncGeneratorFunctionParseMode(parseMode))
> +	       return SourceParseMode::AsyncGeneratorBodyMode;
> +
> +	   if (parseMode == SourceParseMode::AsyncArrowFunctionMode)
> +	       return SourceParseMode::AsyncArrowFunctionBodyMode;
> +	   
> +	   return SourceParseMode::AsyncFunctionBodyMode;
> +    }

Let's put it in Parser.cpp as static function insead of Parser's member
function.