Skip to content

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

Closed
gabrielschulhof wants to merge 1 commit intonodejs:masterfrom
gabrielschulhof:fix-windows-build2
Closed

[napi] properly initialize and check status (#12279)#12283
gabrielschulhof wants to merge 1 commit intonodejs:masterfrom
gabrielschulhof:fix-windows-build2

Conversation

@gabrielschulhof
Copy link
Contributor

@gabrielschulhof gabrielschulhof commented 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.

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

vsemozhetbyt commented Apr 8, 2017

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

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 8, 2017 •
edited
Loading

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
Loading

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

@refack refack Apr 8, 2017

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

@gabrielschulhof gabrielschulhof Apr 8, 2017

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

@refack refack Apr 8, 2017

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
Loading

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
Loading

@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

gabrielschulhof commented Apr 8, 2017

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

gabrielschulhof commented Apr 8, 2017

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

1 more reviewer

@refack refack refack approved these changes

Reviewers whose approvals may not affect merge requirements

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.

7 participants

@gabrielschulhof @vsemozhetbyt @refack @Trott @italoacasas @gibfahn @nodejs-github-bot