test: pass null params to napi_create_function() by octaviansoldea · Pull Request #26998 · nodejs/node (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation11 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Each one of the following arguments is checked:
napi_env env,
const char* utf8name,
napi_callback cb,
napi_value* result.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes- tests and/or benchmarks are included
- commit message follows commit guidelines
Each one of the following arguments is checked: napi_env env, const char* utf8name, napi_callback cb, napi_value* result.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a few nits.
| &result); |
|---|
| ret[1] = napi_create_function(env, |
| NULL, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this and the lines below need a few more spaces of indentation.
| static napi_value TestCreateFunctionParameters(napi_env env, |
|---|
| napi_callback_info info) { |
| napi_status ret[4]; |
| napi_value result, return_value = NULL, prop_value; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: we can forego the initalizer here.
1 similar comment
@BridgeAR I'm OK with that. I'm surprised the linter hasn't caught any of this.
gabrielschulhof pushed a commit that referenced this pull request
Each one of the following arguments is checked: napi_env env, const char* utf8name, napi_callback cb, napi_value* result.
PR-URL: #26998 Reviewed-By: Gabriel Schulhof gabriel.schulhof@intel.com
This was referenced
Apr 23, 2019
octaviansoldea pushed a commit to octaviansoldea/node that referenced this pull request
Trott pushed a commit to Trott/io.js that referenced this pull request
This is a refactoring of nodejs#26998 following nodejs#28505.
The functions add_last_status() and add_returned_status() are now
reused, see also nodejs#28848.
PR-URL: nodejs#28894 Reviewed-By: Gabriel Schulhof gabriel.schulhof@intel.com Reviewed-By: Michael Dawson michael_dawson@ca.ibm.com Reviewed-By: Rich Trott rtrott@gmail.com
targos pushed a commit that referenced this pull request
This is a refactoring of #26998 following #28505.
The functions add_last_status() and add_returned_status() are now
reused, see also #28848.
PR-URL: #28894 Reviewed-By: Gabriel Schulhof gabriel.schulhof@intel.com Reviewed-By: Michael Dawson michael_dawson@ca.ibm.com Reviewed-By: Rich Trott rtrott@gmail.com
Labels
Issues and PRs related to the Node-API.
Issues and PRs related to the tests.