I guess it would be ok to have it return Local<Object> and crash on anything else inside the API call. It would not break the API since existing users of this should just compile just fine.
Accepting patches :) On Thursday, February 11, 2016 at 7:58:06 PM UTC+1, Alex Kodat wrote: > > Would it be a waste of everyone's time if I created an issue and submitted > a change to make Error return Local<Object>? FWIW, my inclination would > make Factory::NewError return a JSObject and go from there though if people > fee that's a bridge too far, I'd just put a checked cast into api.cc. It > just seems kinda crazy for Factor::NewError to happily return to its caller > if the situation is so grim it can't return an error object. In fact, I > kinda wonder about it catching an exception thrown by the native > constructor and returning that to the caller. It's really hard to picture > what would be going on in such a scenario and if there's anything sensible > to do beyond falling on one's sword. > > Or maybe this is all too trivial to bother with in which case I appreciate > the discussion, anyway. > > On Thursday, February 11, 2016 at 4:10:39 AM UTC-8, Yang Guo wrote: >> >> The issue is... with other parts of the API, when we return a MaybeLocal, >> it may have thrown an exception, in which process we created an Error >> object, and return an empty handle as result. >> >> This case is special. We want to create an Error object. If that itself >> fails, throwing an exception makes no sense, since we cannot create another >> Error object. We don't expect this to happen unless bootstrapping hasn't >> finished yet, or we ran out of stack space, or something to that effect. >> >> I guess we could turn that into a MaybeLocal<Object>, but I don't really >> think we should break the API for this small detail. That's just my opinion >> though. >> >> Yang >> >> On Wednesday, February 10, 2016 at 6:39:44 PM UTC+1, Alex Kodat wrote: >>> >>> First, I'll confess I'm not a huge fan of MaybeLocal but, leaving that >>> aside, even if I accept the utility of MaybeLocal I would expect it to be >>> used for errors for which there's a reasonable hope of recovery and some >>> way of actually testing the recovery code. >>> >>> If Exception::Error returns an empty result, the world (or at least the >>> Isolate) has turned to yogurt and I feel I can't rely on any API calls >>> actually working so it seems like there's not much for me to do other than >>> crashing and I would have preferred the crash closer to the point where the >>> construction of Error actually failed (Factory::NewError). Beyond that, >>> short of an extremely artificial test where I say hack the Error native >>> constructors, I can't see how I could possibly test any error recovery code >>> for an Exception::Error failure. To me this sort of error seems closer to >>> out of storage errors than unusual results errors. >>> >>> All that said, I guess MaybeLocal<Object> Exception::Error is better >>> than Local<Value> or MaybeLocal<Value> as it better documents what an >>> embedder should expect though the former might be accompanied by a comment >>> that the ToLocalChecked on the return value should only ever fail in the >>> event of catastrophic errors so you might not want to expend much bandwidth >>> on worrying about it. >>> >>> On Wednesday, February 10, 2016 at 7:55:01 AM UTC-8, Daniel Vogelheim >>> wrote: >>>> >>>> Generally, the API tries hard to pass errors up. >>>> >>>> I wonder if we should return MaybeLocal<Object>, then. There's been a >>>> huge APi refactoring in the past to deprecate returning empty Locals (or >>>> Undefined, or so) as error markers, and instead signal all such failures >>>> by >>>> returning an empty MaybeLocal. Not quite what Alex asks for, but IMHO more >>>> consistent with the remainder of the API. >>>> >>>> >>>> On Wed, Feb 10, 2016 at 4:26 PM, Alex Kodat <[email protected]> >>>> wrote: >>>> >>>>> Thanks for that. I suspected as much. Is v8 really doing embedder's a >>>>> favor by exposing such a catastrophe to them? Presumably, if >>>>> Factory::NewError fails, we're out of storage (which v8 correctly doesn't >>>>> do embedders the favor of exposing to them), there's some other >>>>> catastrophic failure (like say the embedder's code has run amok >>>>> clobbering >>>>> the v8 heap), or the native Error constructors have a problem >>>>> (inconceivable except maybe if a developer was fiddling with them). >>>>> >>>>> None of these seem like cases where it's useful to share the pain with >>>>> the embedder code, especially as the situation is presumably that the >>>>> embedder code wants to reflect an error to the JS but whoops, all it can >>>>> get is an undefined from Exception::Error. Now what? >>>>> >>>>> Could I/should I open an issue for this? Sorry if this is a stupid >>>>> question, I'm still not quite sure when it's appropriate to post to the >>>>> list or open an issue. While I know you can code the change about as fast >>>>> as I can hit the Post button, I'd be happy to make the change myself. >>>>> FWIW, >>>>> I'd do the type-checking and cast in Factory::NewError and have it (them) >>>>> return a JSObject as it seems like it might be useful for other V8 code >>>>> to >>>>> be able to count on NewError giving it an object (like for example, the >>>>> JSON parser?). Maybe this is too trivial to waste anyone's bandwidth on? >>>>> >>>>> On Wednesday, February 10, 2016 at 5:25:29 AM UTC-8, Yang Guo wrote: >>>>>> >>>>>> This probably never happens, but in case creating the error object >>>>>> fails, undefined is returned. >>>>>> >>>>>> On Monday, February 8, 2016 at 9:03:42 PM UTC+1, Alex Kodat wrote: >>>>>>> >>>>>>> This must have been asked before but can't find an explanation so >>>>>>> ... just curious why Exception::Error et al are declared to have a >>>>>>> Local<Value> result instead of Local<Object>. A not uncommon pattern is >>>>>>> to >>>>>>> create a new Error object and then set some properties on it which >>>>>>> requires >>>>>>> a ->ToObject or Local<Object>::Cast on the Exception::Error result. >>>>>>> Trivial, but it just seems odd that it's necessary. >>>>>>> >>>>>>> Thanks >>>>>>> >>>>>> -- >>>>> -- >>>>> v8-users mailing list >>>>> [email protected] >>>>> http://groups.google.com/group/v8-users >>>>> --- >>>>> You received this message because you are subscribed to the Google >>>>> Groups "v8-users" group. >>>>> To unsubscribe from this group and stop receiving emails from it, send >>>>> an email to [email protected]. >>>>> For more options, visit https://groups.google.com/d/optout. >>>>> >>>> >>>> -- -- v8-users mailing list [email protected] http://groups.google.com/group/v8-users --- You received this message because you are subscribed to the Google Groups "v8-users" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
