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.

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

[napi] properly initialize and check status (#12279) #12283

Closed

Conversation

gabrielschulhof
Copy link
Contributor

Initialize status to napi_generic_failure and only check it after
having made an actual N-API call.

This fixes up 8fbace1.

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

n-api

Initialize status to napi_generic_failure and only check it after
having made an actual N-API call.

This fixes up 8fbace1.
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 8, 2017
@gabrielschulhof gabrielschulhof mentioned this pull request Apr 8, 2017
3 tasks
@vsemozhetbyt vsemozhetbyt added the node-api Issues and PRs related to the Node-API. label Apr 8, 2017
@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 8, 2017

A nit concerning commit messge (see guideline). It could be replaced by something like this (but it can be done by someone who will land this PR):

napi: initialize and check status properly

Initialize status to napi_generic_failure and only check it after
having made an actual N-API call.

This fixes up 8fbace163afbd61b5efc57cf94414be904bf0188.

Fixes: https://github.com/nodejs/node/pull/12279

@refack
Copy link
Contributor

refack commented Apr 8, 2017

Fails on my machine 😣 Sorry. Passes on my machine.

@refack
Copy link
Contributor

refack commented Apr 8, 2017

I think it could be considered green, but just in case:
Another CI Job: https://ci.nodejs.org/job/node-test-pull-request/7284/

@@ -2162,7 +2162,7 @@ napi_status napi_instanceof(napi_env env,

if (env->has_instance_available) {
napi_value value, js_result, has_instance = nullptr;
napi_status status;
napi_status status = napi_generic_failure;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why initiate with failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because then everything will come tumbling down if you compare the value without setting it first - on every platform, all the time.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean why not napi_ok (which is 0)?

@refack
Copy link
Contributor

refack commented Apr 8, 2017

Landed in 14749f9
(fasttracked to remove regression)

@refack refack closed this Apr 8, 2017
@refack refack mentioned this pull request Apr 8, 2017
2 tasks
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 8, 2017

@refack Sorry, wrong hash in "Landed in...". And it is better to use full URL in Refs:... But I don't know is it worth to force-push now.

refack pushed a commit that referenced this pull request Apr 8, 2017
Initialize status to napi_generic_failure and only check it after
having made an actual N-API call.

This fixes up 8fbace1.

PR-URL: #12283
Ref: #12279
Reviewed-By: Refael Ackermann <refack@gmail.com>
@refack
Copy link
Contributor

refack commented Apr 8, 2017

Actually landed in afd5966
(did a force push just for the rush 😵)

@Trott
Copy link
Member

Trott commented Apr 8, 2017

Thanks for the quick fix/review/land, @gabrielschulhof @refack & @vsemozhetbyt!

Still a bunch of infrastructure-related stuff going wrong on CI, but hopefully we get this all straightened out soon. Definitely highlights the room for improvement in our process, for sure.

@gabrielschulhof
Copy link
Contributor Author

Thanks, and sorry about the fuss!

@refack
Copy link
Contributor

refack commented Apr 8, 2017

💩 happens...
I guess the MSVC compiler does not initiate stack value types... Important lesson.

@gabrielschulhof
Copy link
Contributor Author

nod

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
Initialize status to napi_generic_failure and only check it after
having made an actual N-API call.

This fixes up 8fbace1.

PR-URL: nodejs#12283
Ref: nodejs#12279
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
Initialize status to napi_generic_failure and only check it after
having made an actual N-API call.

This fixes up 8fbace1.

Backport-PR-URL: #19447
PR-URL: #12283
Ref: #12279
Reviewed-By: Refael Ackermann <refack@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
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.

7 participants