[webkit-reviews] review denied: [Bug 166698] [ESNext] Async iteration - Implement async iteration statement: for-await-of : [Attachment 319132] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <utatane.tea at gmail.com> has denied GSkachkov
<gskachkov at gmail.com>'s request for review:
Bug 166698: [ESNext] Async iteration - Implement async iteration statement:
for-await-of
https://bugs.webkit.org/show_bug.cgi?id=166698

Attachment 319132: Patch

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




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

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

Looks good. But r- due to the bug and the test coverage.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4301
> +    bool isForAwait = forLoopNode != nullptr ? forLoopNode->isForAwait() :
false;

style: we do not use `xxx != nullptr`.
I think this can be `bool isForAwait = forLoopNode &&
forLoopNode->isForAwait();`

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4419
> +	       emitIteratorNext(value.get(), iterator.get(), node, isForAwait ?
JSC::EmitAwait::Yes : JSC::EmitAwait::No);

JSC:: is not necessary.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4432
> +	       emitIteratorClose(iterator.get(), node, isForAwait ?
JSC::EmitAwait::Yes : JSC::EmitAwait::No);

JSC:: is not necessary.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4584
> +RegisterID* BytecodeGenerator::emitIteratorNext(RegisterID* dst, RegisterID*
iterator, const ThrowableExpressionData* node, JSC::EmitAwait doEmitAwait)

JSC:: is not necessary.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:783
> +	   RegisterID* emitIteratorNext(RegisterID* dst, RegisterID* iterator,
const ThrowableExpressionData* node, JSC::EmitAwait = JSC::EmitAwait::No);

JSC:: is not necessary.

> Source/JavaScriptCore/parser/Parser.cpp:1280
> +    }

I think this accidentally allows `for await (;;)`, `for await (let v = 42;;)`
etc.
If `for await` comes, only for-of style is allowed.
And could you add a syntax test to cover cases like this?