[webkit-reviews] review granted: [Bug 166695] [ESNext] Async iteration - Implement Async Generator : [Attachment 317254] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted GSkachkov <gskachkov at gmail.com>'s
request for review:
Bug 166695: [ESNext] Async iteration - Implement Async Generator
https://bugs.webkit.org/show_bug.cgi?id=166695

Attachment 317254: Patch

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




--- Comment #131 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 317254
  --> https://bugs.webkit.org/attachment.cgi?id=317254
Patch

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

This patch is huge. I've read most of it, but not all. Posting comments for
now. r- for now because I found a couple bugs in builtins and Yusuke's
comments.

> Source/JavaScriptCore/ChangeLog:13
> +	   # await - wait until promise will be resolved and than continue

than => then

> Source/JavaScriptCore/ChangeLog:15
> +	   The main difference between async function and async generator that,


generator that => generator is that

> Source/JavaScriptCore/ChangeLog:26
> +	   # The behavior of yield* is modified to support 
> +	     delegation to async iterables 

only inside an async generator?

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:41
> +	       function(result) {  promiseCapability. at resolve.@call(@undefined,
{ done: !!done, value: result }); },

Is this !!done in the spec? I think it may be observable.

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:61
> +	   promiseCapability. at resolve.@call(@undefined, {value: returnValue,
done: true});

style nit: Your object literals are not consistent with having a space around
the after/before "{" "}" or not. I'm not sure what our official style in JSC
is, but we should probably pick something and try to stick with it.

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:127
> +    if (!@isObject(syncIterator)) @throwTypeError('Only objects can be
wrapped by async-from-sync wrapper');

style nit: newline after the if(...)

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:37
> +    return queue.shift();

this is wrong, you need to use private methods here otherwise you're using an
untrusted "shift" method being on ArrayPrototype.

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:102
> +    function asyncGeneratorYieldAwaited(result)
> +    {
> +	   generator. at asyncGeneratorSuspendReason =
@AsyncGeneratorSuspendReasonYield;
> +	   @asyncGeneratorResolve(generator, result, false);
> +    }
> +
> +    generator. at asyncGeneratorSuspendReason =
@AsyncGeneratorSuspendReasonAwait;
> +
> +    @awaitValue(generator, value, asyncGeneratorYieldAwaited);

Why can't you just immediately resolve here? Won't we always have a regular
object we need to resolve? Maybe I'm missing something.

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:114
> +    const onRejected = onRejectedCallback || function(result) {
@doAsyncGeneratorBodyCall(generator, result, @GeneratorResumeModeThrow); };

you only call this function with truthy values. Why the "||"?

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:137
> +	   if (generator. at generatorState === @AsyncGeneratorStateExecuting) {
> +	       generator. at generatorState = @AsyncGeneratorStateCompleted;
> +	   }

style: no braces

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:155
> +    if (generator. at asyncGeneratorSuspendReason ===
@AsyncGeneratorSuspendReasonYield) {
> +	   return @asyncGeneratorYield(generator, value, resumeMode);
> +    }

style: no braces

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:186
> +
> +

style: remove one line here please

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:211
> +    } else if (state === @AsyncGeneratorStateCompleted) {
> +	   return @asyncGeneratorResolve(generator, @undefined, true);
> +    }

style: no braces

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:232
> +    if (generator. at asyncGeneratorQueue === @undefined)
> +	   generator. at asyncGeneratorQueue = [];

Why not do this during construction?

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:234
> +    generator. at asyncGeneratorQueue.push({resumeMode, value,
promiseCapability});

you need to use a private name here.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:64
> +    enum EmitAwait { Yes, No };

style: enum class please

> Source/JavaScriptCore/jit/JITOpcodes.cpp:1043
> +    else if (opcodeID == op_new_async_generator_func_exp)
> +	   callOperation(operationNewAsyncGeneratorFunction, dst, regT0,
function);
> +    else {
> +	   ASSERT(opcodeID == op_new_async_generator_func_exp);
> +	   callOperation(operationNewAsyncGeneratorFunction, dst, regT0,
function);

Ditto to what Yusuke said here

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1185
> +	   dataLogF("Creating async generator function!\n");

style: de-indent 4 spaces

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1300
> +    dispatch(4)

should be: dispatch(constexpr op_new_asyc_generator_func_length)
now that Keith made this possible

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1305
> +    dispatch(4)

ditto

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:683
> +    m_asyncGeneratorFunctionPrototype->putDirectWithoutTransition(vm,
vm.propertyNames->constructor, asyncGeneratorFunctionConstructor, DontEnum |
ReadOnly);

nit: This feels like it should be in AsyncGeneratorFunctionPrototype create.