1328820 - Add documentation comment to Promise.h, AsyncFunction.h, and AsyncIteration.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Tooru Fujisawa [:arai]]

derived from bug 1325606 comment #3.

each function in AsyncFunction.h needs documentation, and also it would be nice to document the structure of async wrapped/unwrapped functions.

Comment 1

[Tooru Fujisawa [:arai]]

Now that we're applying several optimizations, we should have documentation.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Add documentation comment to Promise.h, AsyncFunction.h, and AsyncIteration.h

Added documentation comment for class, fields, and some functions.
I didn't add comment for function which has a comment about spec steps in .cpp file and it was clear enough.

Comment 3

[André Bargull [:anba]]

Comment on attachment 9003352 [details] [diff] [review]
Add documentation comment to Promise.h, AsyncFunction.h, and AsyncIteration.h

Review of attachment 9003352 [details] [diff] [review]:
-----------------------------------------------------------------

Mainly clearing review because of the "unpaired" term. I assume "unpaired" was intended to document that there's a one-to-one relationship between the wrapped and unwrapped function objects of async (generator) functions, but I'm not entirely sure.

::: js/src/builtin/Promise.h
@@ +14,5 @@
>  
>  namespace js {
>  
>  enum PromiseSlots {
> +    // Int32 value wiht PROMISE_FLAG_* flags below.

typo: wiht -> with

@@ +20,5 @@
> +
> +    // * if this promise is pending, reaction objects
> +    //     * undefined if there's no reaction
> +    //     * maybe-wrapped PromiseReactionRecord if there's only one reacion
> +    //     * dense array if there's 2 or more more reactions

s/there's 2/there are two/

typo: "more more" -> more

@@ +27,5 @@
>      PromiseSlot_ReactionsOrResult,
> +
> +    // * if this promise is pending, resolve/reject functions.
> +    //   this slot holds reject function, and resolve function is linked from
> +    //   the reject function's extended slot

"This slot holds only the reject function. The resolve function is reachable from the reject function's extended slot."

@@ +29,5 @@
> +    // * if this promise is pending, resolve/reject functions.
> +    //   this slot holds reject function, and resolve function is linked from
> +    //   the reject function's extended slot
> +    // * if this promise is either fulfilled or rejected, undefined
> +    // * (special case) if this promise is the return value of async function

s/of/of an/

@@ +30,5 @@
> +    //   this slot holds reject function, and resolve function is linked from
> +    //   the reject function's extended slot
> +    // * if this promise is either fulfilled or rejected, undefined
> +    // * (special case) if this promise is the return value of async function
> +    //   invocation, generator object for the function's internal generator

s/generator/the generator/

@@ +36,5 @@
>      PromiseSlot_AwaitGenerator = PromiseSlot_RejectFunction,
> +
> +    // Promise object's debug info, which is created on demand.
> +    // * if this promise has no debug info, undefined
> +    // * if this promise has only id, number value for the id

s/has only id/contains only its process-unique ID, the ID's number value/

@@ +37,5 @@
> +
> +    // Promise object's debug info, which is created on demand.
> +    // * if this promise has no debug info, undefined
> +    // * if this promise has only id, number value for the id
> +    // * if this promise has entire debug info, PromiseDebugInfo

"otherwise a PromiseDebugInfo object"

@@ +44,4 @@
>      PromiseSlots,
>  };
>  
> +// This promise is either fulfilled/rejected.

s/\rejected/or rejected/

@@ +49,4 @@
>  #define PROMISE_FLAG_RESOLVED  0x1
> +
> +// If this flag and PROMISE_FLAG_RESOLVED are set, this promise is fulfilled.
> +// If this flag is not set while PROMISE_FLAG_RESOLVED is set, it's rejected.

"// If only this flag is set, this promise is rejected."

@@ +53,4 @@
>  #define PROMISE_FLAG_FULFILLED 0x2
> +
> +// This promise resolution, especially rejection, is handled by `.then` etc.
> +// Embedding is supposed to report unhandled rejection.

Maybe just copy something along the lines of https://tc39.github.io/ecma262/#sec-properties-of-promise-instances:
---
Indicates the promise has ever had a fulfillment or rejection handler; used in unhandled rejection tracking. 
---

@@ +57,3 @@
>  #define PROMISE_FLAG_HANDLED   0x4
> +
> +// This promise uses default resolving functions and PromiseSlot_RejectFunction

s/default/the default/

s/functions and PromiseSlot_RejectFunction/functions. The PromiseSlot_RejectFunction/

@@ +61,3 @@
>  #define PROMISE_FLAG_DEFAULT_RESOLVING_FUNCTIONS 0x08
> +
> +// This promise is the return value of async function invocation.

s/async/an async/

@@ +126,5 @@
>          return resolutionTime() - allocationTime();
>      }
>      MOZ_MUST_USE bool dependentPromises(JSContext* cx, MutableHandle<GCVector<Value>> values);
> +
> +    // Return the ID of this promise for debugging purpose.

"// Return the process-unique ID of this promise. Only used by the debugger."

@@ +190,5 @@
>  PromiseResolve(JSContext* cx, HandleObject constructor, HandleValue value);
>  
>  
> +/**
> + * Create a promise object that is the return value of async function.

"Create the promise object which will be used as the return value of an async function."

@@ +191,5 @@
>  
>  
> +/**
> + * Create a promise object that is the return value of async function.
> + * This promise object holds the generator object for later use.

I'd probably just delete this sentence, because "for later use" is too nondescript.

::: js/src/vm/AsyncFunction.h
@@ +15,5 @@
> +// Async function consists of 2 functions, |wrapped| and |unwrapped|.
> +// |unwrapped| is a generator function compiled from async function script,
> +// |await| behaves just like |yield| there.  |unwrapped| isn't exposed to user
> +// script.
> +// |wrapped| is a native function that is the value of async function.

I'd rewrite this more like:
---
An async function is implemented using two function objects, which are referred to as the "unwrapped" and the "wrapped" async function object.
The unwrapped function is a generator function compiled from the async function's script. |await| expressions within the async function are translated into |yield| expression for the generator function. The unwrapped function is never exposed to user script.
The wrapped function is a native function which wraps the generator function, hence its name, and is the publicly exposed object of the async function.
---

@@ +17,5 @@
> +// |await| behaves just like |yield| there.  |unwrapped| isn't exposed to user
> +// script.
> +// |wrapped| is a native function that is the value of async function.
> +
> +// Returns wrapped async function from unwrapped async function.

s/wrapped/the wrapped/
s/unwrapped/an unwrapped/

@@ +22,4 @@
>  JSFunction*
>  GetWrappedAsyncFunction(JSFunction* unwrapped);
>  
> +// Returns unwrapped async function from wrapped async function.

Ditto.

@@ +26,4 @@
>  JSFunction*
>  GetUnwrappedAsyncFunction(JSFunction* wrapped);
>  
> +// Returns true if the given function is wrapped async function.

s/is/is a/

@@ +30,5 @@
>  bool
>  IsWrappedAsyncFunction(JSFunction* fun);
>  
> +// Create a wrapped async function from unpaired unwrapped async function
> +// with given prototype object.

"unpaired" again

(I've started reviewing from AsyncIteration.h, so my review comments most certainly end up in the wrong order.)

@@ +35,5 @@
>  JSObject*
>  WrapAsyncFunctionWithProto(JSContext* cx, HandleFunction unwrapped, HandleObject proto);
>  
> +// Create a wrapped async function from unpaired unwrapped async function
> +// with default prototype object.

"unpaired"

s/with/with the/

@@ +40,5 @@
>  JSObject*
>  WrapAsyncFunction(JSContext* cx, HandleFunction unwrapped);
>  
> +// Resume the async function when the `await` operand resolves, each for
> +// fulfilled case and rejected case.

Same issue with "each for ... case" as in AsyncIteration.h.

::: js/src/vm/AsyncIteration.h
@@ +19,5 @@
>  // |await| behaves just like |yield| there.  |unwrapped| isn't exposed to user
>  // script.
>  // |wrapped| is a native function that is the value of async generator.
>  
> +// Create a wrapped async generator from unpaired unwrapped async generator

I don't understand "unpaired" here. Can you clarify?

@@ +25,5 @@
>  JSObject*
>  WrapAsyncGeneratorWithProto(JSContext* cx, HandleFunction unwrapped, HandleObject proto);
>  
> +// Create a wrapped async generator from unpaired unwrapped async generator
> +// with default prototype object.

Also "unpaired".

@@ +30,4 @@
>  JSObject*
>  WrapAsyncGenerator(JSContext* cx, HandleFunction unwrapped);
>  
> +// Returns true if the given function is wrapped async generator.

Nit: "a" between "is" and "wrapped".

@@ +34,4 @@
>  bool
>  IsWrappedAsyncGenerator(JSFunction* fun);
>  
> +// Returns wrapped async generator from unwrapped async generator.

Nit: "the" before "wrapped" and "an" before "unwrapped".

@@ +38,4 @@
>  JSFunction*
>  GetWrappedAsyncGenerator(JSFunction* unwrapped);
>  
> +// Return unwrapped async generator from wrapped async generator.

Ditto.

@@ +53,5 @@
>                                HandleValue reason);
> +
> +// Resume the async generator after awaiting on the value passed to
> +// AsyncGenerator#return, when the async generator was still executing.
> +// Each for fullfilled case and rejected case.

"Each for fullfilled case and rejected case." is probably grammatically not correct. Maybe "Split into two functions depending on whether the awaited value was fulfilled or rejected." or something like that.

@@ +68,5 @@
>  
> +// AsyncGeneratorRequest record in the spec.
> +// Stores the info from AsyncGenerator#{next,return,throw}.
> +//
> +// This object is reused across multiple request as an optimization, stored in

s/request/requests/

s/, stored in Slot_CachedRequest slot/and stored in the Slot_CachedRequest slot/

@@ +125,5 @@
>  class AsyncGeneratorObject : public NativeObject
>  {
>    private:
>      enum AsyncGeneratorObjectSlots {
> +        // Int32 value with State enum below.

"Int32 value containing one of the |State| fields from below." or some such.

@@ +130,3 @@
>          Slot_State = 0,
> +
> +        // Generator object for unwrapped async generator.

s/for/for this/

@@ +133,3 @@
>          Slot_Generator,
> +
> +        // * null value if this async generator has no request

s/request/requests/

@@ +322,5 @@
>  class AsyncFromSyncIteratorObject : public NativeObject
>  {
>    private:
>      enum AsyncFromSyncIteratorObjectSlots {
> +        // Object that implementes sync iterator protocol.

typo: "implementes" -> "implements"

And "the" before "sync".

@@ +327,3 @@
>          Slot_Iterator = 0,
> +
> +        // The `next` property ob the iterator object.

Typo "ob" -> "of"

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Add documentation comment to Promise.h, AsyncFunction.h, and AsyncIteration.h

Thank you for reviewing :)

I guess I chose wrong word.
removed "unpaired" and added a comment about how wrapped and unwrapped functions are created.
and yes, there's one-to-one relationship. unwrapped function is created first, while compiling. Until execution, it doesn't have corresponding wrapped function.
when evaluating async function (not body), wrapped function is created and they're linked each other.

Comment 5

[André Bargull [:anba]]

Comment on attachment 9003993 [details] [diff] [review]
Add documentation comment to Promise.h, AsyncFunction.h, and AsyncIteration.h

Review of attachment 9003993 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!

::: js/src/vm/AsyncIteration.h
@@ +78,5 @@
> +// Stores the info from AsyncGenerator#{next,return,throw}.
> +//
> +// This object is reused across multiple requests as an optimization, and
> +// stored in the Slot_CachedRequest.
> +slot/

Belongs in the previous line. :-)

Comment 6

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b205a608fff3adb169a081318f3207a3ae1d507d
Bug 1328820 - Add documentation comment to Promise.h, AsyncFunction.h, and AsyncIteration.h r=anba DONTBUILD comment-only

Comment 7

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/b205a608fff3