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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

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

[napi] properly initialize and check status (nodejs#12279)
3b0d344 
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
Closed
3 tasks
@vsemozhetbyt vsemozhetbyt added the node-api Issues and PRs related to the Node-API. label Apr 8, 2017
@vsemozhetbyt
Copy link
Contributor

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

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 8, 2017 •
edited

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 •
edited

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/

refack
refack reviewed Apr 8, 2017
View reviewed changes
src/node_api.cc
@@ -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 •
edited

Landed in 14749f9
(fasttracked to remove regression)

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

vsemozhetbyt commented Apr 8, 2017 •
edited

@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
@refack
napi: initialize and check status properly
afd5966 
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

@refack refack mentioned this pull request Apr 10, 2017
Closed
3 tasks
@jasnell jasnell mentioned this pull request May 11, 2017
Closed
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
napi: initialize and check status properly
1459aea 
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
@MylesBorins
napi: initialize and check status properly
a6af97f 
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
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

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.

None yet
7 participants
@gabrielschulhof @vsemozhetbyt @refack @Trott @italoacasas @gibfahn @nodejs-github-bot