Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

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 Qard, addaleax and mhdawson and removed request for addaleax November 13, 2020 21:21
src/js_native_api_v8.h Outdated Show resolved Hide resolved
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

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.

Remove semicolon from macro definition

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

gabrielschulhof pushed a commit that referenced this pull request Nov 16, 2020
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
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
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
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
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
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
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

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

Development

Successfully merging this pull request may close these issues.

6 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.