src: set default return value of Reference Ref/Unref to 0#1004
src: set default return value of Reference Ref/Unref to 0#1004legendecas wants to merge 1 commit into
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 :( |
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>
2 participants
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)