[webkit-reviews] review denied: [Bug 175891] [ESNext] Async iteration - Implement Async Generator - optimization : [Attachment 320460] 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 175891: [ESNext] Async iteration - Implement Async Generator - optimization
https://bugs.webkit.org/show_bug.cgi?id=175891

Attachment 320460: Patch

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




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

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

I think the current one has a bug. But the direction looks very nice to me.

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:36
>  {

This function is unnecessary.

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:40
> +    value.next = null;
> +    value.previous = previous;

I think this is not safe. Users can trap this by setting
Object.prototype.{next,previous} setters.
When writing builtins, you need to take care of property setters. Basically,
creating a new property is always dangerous (except for @privateProperty).

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:46
> +function asyncGeneratorQueueEnqueue(generator, value)

Let's rename `value` to `item`.

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:51
> +	   generator. at asyncGeneratorQueueFirst =
@asyncGeneratorQueueCreateItem(value, null);

Change it to `generator. at asyncGeneratorQueueFirst = item;`. item.previous
should be null when passing asyncGeneratorQueueEnqueue().
And let's insert `@assert(item.previous === null);` and `@assert(item.next ===
null);`.

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:54
> +	   const item = @asyncGeneratorQueueCreateItem(value,
generator. at asyncGeneratorQueueLast);

Change this to,

`item.previous = generator. at asyncGeneratorQueueLast;`.

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:275
> +    @asyncGeneratorQueueEnqueue(generator, {resumeMode, value,
promiseCapability});

Set `next` and `previous` fields with `null` for this `item`.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2889
> +    emitDirectPutById(m_generatorRegister,
propertyNames().builtinNames().asyncGeneratorQueueLastPrivateName(),
emitLoad(nullptr, jsNull()), PropertyNode::KnownDirect);

Nice.