-
Notifications
You must be signed in to change notification settings - Fork 30k
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
doc: fix up N-API doc #30254
doc: fix up N-API doc #30254
Conversation
* Add missing N-API version info * Fix N-API version info for napi_extended_error_info
@NickNaso let me know if I missed anything. |
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.
@mhdawson I think that you missed only napi_add_finalizer
it's added in Node.js version 8.0.0 but it's promoted as stable in N-API version 5.
@mhdawson I found some other inconsistencies that I report below:
NAPI_EXTERN napi_status napi_create_reference(napi_env env,
napi_value value,
int initial_refcount,
napi_ref* result); instead it should be: NAPI_EXTERN napi_status napi_create_reference(napi_env env,
napi_value value,
uint32_t initial_refcount,
napi_ref* result);
NAPI_EXTERN napi_status napi_reference_ref(napi_env env,
napi_ref ref,
int* result); instead it should be: NAPI_EXTERN napi_status napi_reference_ref(napi_env env,
napi_ref ref,
uint32_t* result);
NAPI_EXTERN napi_status napi_reference_unref(napi_env env,
napi_ref ref,
uint32_t* result);
napi_status napi_call_function(napi_env env,
napi_value recv,
napi_value func,
int argc,
const napi_value* argv,
napi_value* result) instead it should be: NAPI_EXTERN napi_status napi_call_function(napi_env env,
napi_value recv,
napi_value func,
size_t argc,
const napi_value* argv,
napi_value* result);
napi_status napi_make_callback(napi_env env,
napi_async_context async_context,
napi_value recv,
napi_value func,
int argc,
const napi_value* argv,
napi_value* result) instead it should be: NAPI_EXTERN napi_status napi_make_callback(napi_env env,
napi_async_context async_context,
napi_value recv,
napi_value func,
size_t argc,
const napi_value* argv,
napi_value* result); I think that we could avoid to open other PR and handle the necessary changes in this one. |
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.
RSLGTM
@NickNaso thanks I'll look at the non-version related inconsistencies separately. EDIT, I see the comment above, will integrate into this PR. |
@NickNaso napi_add_finalizer was already tagged as version 5 that is why it does not show up in the diffs. |
@NickNaso addressed the rest of the comments |
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
* Add missing N-API version info * Fix N-API version info for napi_extended_error_info PR-URL: #30254 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jiawen Geng <[email protected]>
Landed in 1e0679e |
* Add missing N-API version info * Fix N-API version info for napi_extended_error_info PR-URL: #30254 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jiawen Geng <[email protected]>
* Add missing N-API version info * Fix N-API version info for napi_extended_error_info PR-URL: #30254 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jiawen Geng <[email protected]>
* Add missing N-API version info * Fix N-API version info for napi_extended_error_info PR-URL: #30254 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jiawen Geng <[email protected]>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes