n-api: use Maybe version of SetPrototype #13513
Conversation
Currently the following two warnings are displayed when compiling:
../src/node_api.cc:1966:12: warning: 'SetPrototype' is deprecated
[-Wdeprecated-declarations]
wrapper->SetPrototype(proto);
^
../src/node_api.cc:1967:8: warning: 'SetPrototype' is deprecated
[-Wdeprecated-declarations]
obj->SetPrototype(wrapper);
This commit changes these calls to use the Maybe<bool> version.
There was a problem hiding this comment.
LGTM, assuming it's established that SetPrototype cannot ever return an empty value.
|
cc/ @nodejs/n-api |
| wrapper->SetPrototype(proto); | ||
| obj->SetPrototype(wrapper); | ||
| wrapper->SetPrototype(context, proto).ToChecked(); | ||
| obj->SetPrototype(context, wrapper).ToChecked(); |
There was a problem hiding this comment.
This looks wrong to me; not your changes, but the logic itself. What happens when proto->IsNull() is true? What happens when obj is a proxy object?
There was a problem hiding this comment.
What happens when proto->IsNull() is true?
That should be fine. If it was correct for the original object to have a null prototype, then it is correct for the object inserted in the prototype chain to have a null prototype.
What happens when obj is a proxy object?
Is it not valid to set the prototype of a proxy object? If V8 doesn't allow it, then the SetPrototype() call will presumably return an empty Maybe, and this code should check for that and return an error status in that case.
There was a problem hiding this comment.
@jasongin Thanks for the details. @bnoordhuis Does this seems reasonable to you too?
There was a problem hiding this comment.
@bnoordhuis If you get a chance could you take a look and see if you think I can land this, thanks!
There was a problem hiding this comment.
The point about proxy objects still stand. Since they can intercept (and fail) almost any operation, you have to be prepared for exceptions.
There was a problem hiding this comment.
Ah I see. Let me take a look and come up with a suggestion. Thanks
There was a problem hiding this comment.
@danbev Any luck? That warning irritates me more by the day. Happy to take over if you don't have time.
There was a problem hiding this comment.
@bnoordhuis Sorry, I've not had time yet so please take over if you do have time. Thanks
|
#14053 landed, I'll close this. |
Currently the following two warnings are displayed when compiling:
This commit changes these calls to use the Maybe version.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
n-api