On Fri, Feb 28, 2020 at 2:41 AM Jakob Kummerow <[email protected]> wrote:
> 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. > > The existing code only ever stores a Local<Value> using SetInternalField but given that the Promise we're storing into can come from anywhere the idea is to be defensive and check first that the internal field is not already used. Thinking about it more, I think we can get by with a simple existence check, something like IsAlignedPointerInInternalField(index) that returns a bool or Maybe<bool> would work just fine. - James > (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 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/CAKSzg3RDPzKn0da%3D623VKg2CaPBC1k0xtETVbAmJngTHifX6Zw%40mail.gmail.com > <https://groups.google.com/d/msgid/v8-dev/CAKSzg3RDPzKn0da%3D623VKg2CaPBC1k0xtETVbAmJngTHifX6Zw%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/CABP7RbdULr8A7zrHMGtGsxHfCzbw20bfVJVBazEVr%2Ba0og4eGw%40mail.gmail.com.
