Skip to content
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.

Sign up for GitHub

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

Jump to bottom

n-api: factor out calling pattern #36113

Closed
wants to merge 3 commits into from
Closed

n-api: factor out calling pattern #36113

wants to merge 3 commits into from

Conversation

gabrielschulhof
Copy link
Contributor

Factor out how we handle a napi_status-valued return internally.

Signed-off-by: Gabriel Schulhof gabriel.schulhof@intel.com

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

n-api: factor out calling pattern
58e9258 
Factor out how we handle a `napi_status`-valued return internally.

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/n-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. labels Nov 13, 2020
@gabrielschulhof gabrielschulhof requested review from addaleax, Qard and mhdawson and removed request for addaleax November 13, 2020 21:21
addaleax
addaleax reviewed Nov 15, 2020
View reviewed changes
src/js_native_api_v8.h Outdated Show resolved Hide resolved
legendecas
legendecas reviewed Nov 15, 2020
View reviewed changes
src/js_native_api_v8.cc
v8impl::V8NameFromPropertyDescriptor(env, p, &property_name);

if (status != napi_ok) {
return napi_set_last_error(env, status);
Copy link
Member

@legendecas legendecas Nov 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

STATUS_CALL simply returns on non-napi_ok without calling napi_set_last_error. The behavior seems not equivalent with the change. Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@legendecas yes, it is, because it is assumed that whichever call returned non-napi_ok did so via return napi_set_last_error(napi_<not_ok>); I checked to make sure that was the case in all locations where I made the change.

@addaleax
fixup! Update src/js_native_api_v8.h
8e4b575 
Remove semicolon from macro definition

Co-authored-by: Anna Henningsen <github@addaleax.net>
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/34408/

gabrielschulhof pushed a commit that referenced this pull request Nov 16, 2020
n-api: factor out calling pattern
2fd2235 
Factor out how we handle a `napi_status`-valued return internally.

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: #36113
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@gabrielschulhof
Copy link
Contributor Author

Landed in 2fd2235.

@gabrielschulhof gabrielschulhof deleted the status-call branch November 16, 2020 05:52
codebytere pushed a commit that referenced this pull request Nov 22, 2020
@codebytere
n-api: factor out calling pattern
89da0c3 
Factor out how we handle a `napi_status`-valued return internally.

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: #36113
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Nov 22, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
@BethGriggs
n-api: factor out calling pattern
d679570 
Factor out how we handle a `napi_status`-valued return internally.

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: #36113
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
@BethGriggs
n-api: factor out calling pattern
9282b45 
Factor out how we handle a `napi_status`-valued return internally.

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: #36113
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
@BethGriggs
n-api: factor out calling pattern
08657e7 
Factor out how we handle a `napi_status`-valued return internally.

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: #36113
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@legendecas legendecas legendecas left review comments

@addaleax addaleax addaleax left review comments

@Trott Trott Trott approved these changes

@Qard Qard Qard approved these changes

@mhdawson mhdawson Awaiting requested review from mhdawson

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@gabrielschulhof @nodejs-github-bot @Qard @Trott @addaleax @legendecas