I am not an API owner, so I can only offer some generic background here: generally, we use Maybe and MaybeLocal for things that can fail when a JavaScript exception is thrown, which in particular means that these failures can happen even when the C++ code is perfectly bug-free. (This ties in to the invariant that Yuki pointed out: "maybe.IsNothing()" should be true iff "i::isolate->has_pending_exception()".) CHECKs and DCHECKs, on the other hand, are used for things that are not supposed to ever happen if the C++ code is correct.
GetAlignedPointerFromInternalField has traditionally been seen as being in the latter bucket: if an embedder calls that, and the field does not contain an aligned pointer, then that embedder's code is incorrect and *should* crash. It sounds to me like what you're doing is you're using the same internal field to sometimes store an aligned pointer and sometimes store a heap object, is that correct? The cleanest solution, given the existing architecture, would probably be to use separate fields. (As for your naming question: a common pattern is to use names like TryGetFoo; but considering the above I'm not sure a Maybe-typed interface is the way to go in this case.) On Fri, Feb 28, 2020 at 7:19 AM James M Snell <[email protected]> wrote: > Ok! Thank you for the feedback! Very much appreciated > > On Thu, Feb 27, 2020, 21:59 Yuki Shiino <[email protected]> wrote: > >> I don't have a strong opinion except for the convention of throwing an >> exception. >> >> >> 2020年2月27日(木) 23:57 James Snell <[email protected]>: >> >>> Hmm... as far as I know base::Optional is not currently used anywhere >>> else in the public v8 API correct? >>> >>> An alternative here that avoids the exception issue could be: >>> >>> 1. For GetInternalFieldMaybe(), if the value is not set, it returns >>> Undefined rather than empty. Not ideal but certainly something we can work >>> with. >>> 2. For GetAlignedPointerFromInternalField() we could use a pattern more >>> like To() ... that is: >>> >>> bool GetAlignedPointerFromInternalField(int field, void* ptr) >>> >>> Such that if the internal field is an aligned pointer, ptr is set >>> and true is returned, otherwise ptr is untouched and false is returned. >>> With no assert. It's less ideal but also something we can work with. >>> >>> - James >>> >>> On Wednesday, February 26, 2020 at 10:00:53 PM UTC-8, Yuki Shiino wrote: >>>> >>>> Blink-V8 bindings team expects (and is relying on) a convention that, >>>> if a Maybe/MaybeLocal is empty, then an exception is being thrown. This is >>>> very important when implementing V8 callback functions (IDL attribute >>>> callback, IDL operation callback, etc.). >>>> >>>> v8::MaybeLocal<v8::Value> maybe_value = Func(); >>>> v8::Local<v8::Value> value; >>>> if (!maybe_value.ToLocal(&value)) { >>>> return; // Just returns because an exception must be thrown already. >>>> } >>>> // Use |value|. >>>> >>>> >>>> I personally would like V8 APIs to keep this convention. In case of >>>> GetAlignedPointerFromInternalField, I'm afraid that there wouldn't be a >>>> proper exception to be thrown. Maybe, base::Optional<T> would fit better? >>>> >>>> Cheers, >>>> Yuki Shiino >>>> >>>> >>>> 2020年2月27日(木) 8:20 James Snell <[email protected]>: >>>> >>>>> Currently existing value checks from GetInternalField and >>>>> GetAlignedPointerFromInternalField can be a bit awkward. The former >>>>> returns >>>>> a Local<Value>, while the latter returns a void* and asserts if the value >>>>> is not an aligned pointer. >>>>> >>>>> I would like to explore adding a pair of alternatives that would >>>>> return a MaybeLocal<Value> and Maybe<T>, respectively... for instance: >>>>> >>>>> ``` >>>>> MaybeLocal<Value> ret = obj->GetInternalFieldMaybe(0); >>>>> if (!ret.IsEmpty()) { >>>>> Local<Value> val = ret.ToLocalChecked(); >>>>> // do something with val >>>>> } >>>>> ``` >>>>> >>>>> and >>>>> >>>>> ``` >>>>> Foo* foo = nullptr; >>>>> Maybe<Foo> ret = obj->GetAlignedPointerFromInternalFieldMaybe(0); >>>>> if (ret.To(foo)) >>>>> // do something with foo >>>>> ``` >>>>> >>>>> A key difference in behavior with the >>>>> GetAlignedPointerFromInternalFieldMaybe variant is that instead of >>>>> asserting when not an aligned pointer, it would return a Maybe with >>>>> IsNothing == true. One use case for this we have in Node.js is that we >>>>> currently use an internal field in the Promise object to store a reference >>>>> to a tracking object (part of our async context tracking mechanism). That >>>>> is stored using SetInternalField, but we have a check just prior to make >>>>> sure there's not an aligned pointer set in that field already, which will >>>>> assert if SetInternalField has already been called. >>>>> >>>>> Before going off and implementing, I wanted to (a) double check to >>>>> make sure this even made sense and (b) ask for alternative naming >>>>> suggestions because `GetInternalFieldMaybe()` really isn't that great >>>>> unless I was Carly Rae Jepsen. >>>>> >>>>> - James >>>>> >>>>> -- >>>>> -- >>>>> v8-dev mailing list >>>>> [email protected] >>>>> http://groups.google.com/group/v8-dev >>>>> --- >>>>> You received this message because you are subscribed to the Google >>>>> Groups "v8-dev" group. >>>>> To unsubscribe from this group and stop receiving emails from it, send >>>>> an email to [email protected]. >>>>> To view this discussion on the web visit >>>>> https://groups.google.com/d/msgid/v8-dev/8d6fa27c-87cc-422d-8fb1-cf8e34fefa95%40googlegroups.com >>>>> <https://groups.google.com/d/msgid/v8-dev/8d6fa27c-87cc-422d-8fb1-cf8e34fefa95%40googlegroups.com?utm_medium=email&utm_source=footer> >>>>> . >>>>> >>>> -- >>> -- >>> v8-dev mailing list >>> [email protected] >>> http://groups.google.com/group/v8-dev >>> --- >>> You received this message because you are subscribed to the Google >>> Groups "v8-dev" group. >>> To unsubscribe from this group and stop receiving emails from it, send >>> an email to [email protected]. >>> To view this discussion on the web visit >>> https://groups.google.com/d/msgid/v8-dev/e811b00c-d173-4c7f-8a7a-c52d0676cd50%40googlegroups.com >>> <https://groups.google.com/d/msgid/v8-dev/e811b00c-d173-4c7f-8a7a-c52d0676cd50%40googlegroups.com?utm_medium=email&utm_source=footer> >>> . >>> >> -- >> -- >> v8-dev mailing list >> [email protected] >> http://groups.google.com/group/v8-dev >> --- >> You received this message because you are subscribed to a topic in the >> Google Groups "v8-dev" group. >> To unsubscribe from this topic, visit >> https://groups.google.com/d/topic/v8-dev/fk2WLvhONuk/unsubscribe. >> To unsubscribe from this group and all its topics, send an email to >> [email protected]. >> To view this discussion on the web visit >> https://groups.google.com/d/msgid/v8-dev/CAN0uC_Sx5i2sZM4eB%3DRJM%2BdDeL2e96ANor3KbaLgjOS8%2B6UwDQ%40mail.gmail.com >> <https://groups.google.com/d/msgid/v8-dev/CAN0uC_Sx5i2sZM4eB%3DRJM%2BdDeL2e96ANor3KbaLgjOS8%2B6UwDQ%40mail.gmail.com?utm_medium=email&utm_source=footer> >> . >> > -- > -- > v8-dev mailing list > [email protected] > http://groups.google.com/group/v8-dev > --- > You received this message because you are subscribed to the Google Groups > "v8-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > To view this discussion on the web visit > https://groups.google.com/d/msgid/v8-dev/CABP7RbdHYqbOG7aGQ1v6qvo8V2KBqv8UPJaXHoTrLhisd9f-Gw%40mail.gmail.com > <https://groups.google.com/d/msgid/v8-dev/CABP7RbdHYqbOG7aGQ1v6qvo8V2KBqv8UPJaXHoTrLhisd9f-Gw%40mail.gmail.com?utm_medium=email&utm_source=footer> > . > -- -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/CAKSzg3RDPzKn0da%3D623VKg2CaPBC1k0xtETVbAmJngTHifX6Zw%40mail.gmail.com.
