-
Notifications
You must be signed in to change notification settings - Fork 459
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
src: set default return value of Reference Ref/Unref to 0 #1004
Conversation
I'm not sure returning 1 instead of 0 when an error occurs really helps. What would make more sense to me is -1 to indicate an error. If we are going to change the return value, which would affect code that is not properly checking for an exception. I think -1 would be a better value going forward. From the original post where an issue was reported: I agree that calling count = this->Unref(); in the destructor does not make sense since if you count on that decrementing the ref count it will never happen except during env teardown and in that case it is not needed anyway. So the fix for the original issue reported I think is to just remove the Unref() in the ~MyObject() destructor. |
The reference count type is |
@legendecas right did not see it being uint32_t :( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
PR-URL: #1004 Reviewed-By: Michael Dawson <midawson@redhat.com>
Landed as 37a9b8e |
PR-URL: nodejs#1004 Reviewed-By: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs#1004 Reviewed-By: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs/node-addon-api#1004 Reviewed-By: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs/node-addon-api#1004 Reviewed-By: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs/node-addon-api#1004 Reviewed-By: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs/node-addon-api#1004 Reviewed-By: Michael Dawson <midawson@redhat.com>
Update the default value to reflect the most possible values when there are any errors occurred on
napi_reference_unref
.See: https://github.com/nodejs/node/blob/master/src/js_native_api_v8.cc#L2535
Related: #998 (comment)