Skip to content

Fix napi warnings #14053

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
Merged
merged 2 commits into from
Jul 5, 2017
Merged

Fix napi warnings #14053

merged 2 commits into from
Jul 5, 2017

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Jul 3, 2017 •
edited
Loading

Continuation of #13513. I opted for the simplest solution: CHECK all the things.

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

@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 Jul 3, 2017
targos
targos approved these changes Jul 3, 2017
View reviewed changes
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

LGTM with a suggestion

src/node_api.cc Outdated
wrapper->SetPrototype(proto);
obj->SetPrototype(wrapper);
CHECK(wrapper->SetPrototype(context, proto).ToChecked());
CHECK(obj->SetPrototype(context, wrapper).ToChecked());
Copy link
Member

Choose a reason for hiding this comment

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

nit: FromJust() is used in this file but we are not consistent elsewhere so I don't mind.

@targos
Copy link
Member

targos commented Jul 3, 2017

Should we consider turning those warnings into errors? I suppose it's not possible until we remove the v8::Debug stuff.

@bnoordhuis bnoordhuis force-pushed the fix-napi-warnings branch from b219c65 to 439ca22 Compare July 5, 2017 12:37
@bnoordhuis
Copy link
Member Author

Updated with Michaël's suggestion. New CI: https://ci.nodejs.org/job/node-test-pull-request/8987/

Should we consider turning those warnings into errors? I suppose it's not possible until we remove the v8::Debug stuff.

Yes, we can't do that right now unless we get creative with #pragmas to suppress those warnings.

I wouldn't advocate turning on -Werror in release tarballs but it would be good for CI.

bnoordhuis added 2 commits July 5, 2017 16:51
@bnoordhuis
n-api: use Maybe version of Object::SetPrototype()
c9b6d95 
Fixes the following deprecation warning:

    ../src/node_api.cc:2020:30: warning: 'bool
    v8::Object::SetPrototype(v8::Local<v8::Value>)' is deprecated: Use
    maybe version [-Wdeprecated-declarations]
       wrapper->SetPrototype(proto);
    ../src/node_api.cc:2021:28: warning: 'bool
    v8::Object::SetPrototype(v8::Local<v8::Value>)' is deprecated: Use
    maybe version [-Wdeprecated-declarations]
       obj->SetPrototype(wrapper);

PR-URL: nodejs#14053
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@bnoordhuis
n-api: fix -Wmaybe-uninitialized compiler warning
e36917b 
Not an actual bug, as far as I can tell, the compiler is simply not
smart enough to figure out that the offending code path isn't reached
with an uninitialized value.

PR-URL: nodejs#14053
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@bnoordhuis bnoordhuis force-pushed the fix-napi-warnings branch from 439ca22 to e36917b Compare July 5, 2017 14:53
@bnoordhuis bnoordhuis closed this Jul 5, 2017
@bnoordhuis bnoordhuis deleted the fix-napi-warnings branch July 5, 2017 14:54
@bnoordhuis bnoordhuis merged commit e36917b into nodejs:master Jul 5, 2017
@bnoordhuis bnoordhuis mentioned this pull request Jul 6, 2017
Closed
2 tasks
addaleax pushed a commit that referenced this pull request Jul 11, 2017
@bnoordhuis @addaleax
n-api: use Maybe version of Object::SetPrototype()
756190f 
Fixes the following deprecation warning:

    ../src/node_api.cc:2020:30: warning: 'bool
    v8::Object::SetPrototype(v8::Local<v8::Value>)' is deprecated: Use
    maybe version [-Wdeprecated-declarations]
       wrapper->SetPrototype(proto);
    ../src/node_api.cc:2021:28: warning: 'bool
    v8::Object::SetPrototype(v8::Local<v8::Value>)' is deprecated: Use
    maybe version [-Wdeprecated-declarations]
       obj->SetPrototype(wrapper);

PR-URL: #14053
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
@bnoordhuis @addaleax
n-api: fix -Wmaybe-uninitialized compiler warning
6f0a421 
Not an actual bug, as far as I can tell, the compiler is simply not
smart enough to figure out that the offending code path isn't reached
with an uninitialized value.

PR-URL: #14053
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@addaleax addaleax mentioned this pull request Jul 11, 2017
Merged
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@bnoordhuis @addaleax
n-api: use Maybe version of Object::SetPrototype()
de744ba 
Fixes the following deprecation warning:

    ../src/node_api.cc:2020:30: warning: 'bool
    v8::Object::SetPrototype(v8::Local<v8::Value>)' is deprecated: Use
    maybe version [-Wdeprecated-declarations]
       wrapper->SetPrototype(proto);
    ../src/node_api.cc:2021:28: warning: 'bool
    v8::Object::SetPrototype(v8::Local<v8::Value>)' is deprecated: Use
    maybe version [-Wdeprecated-declarations]
       obj->SetPrototype(wrapper);

PR-URL: #14053
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@bnoordhuis @addaleax
n-api: fix -Wmaybe-uninitialized compiler warning
f393304 
Not an actual bug, as far as I can tell, the compiler is simply not
smart enough to figure out that the offending code path isn't reached
with an uninitialized value.

PR-URL: #14053
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
@bnoordhuis @Fishrock123
n-api: use Maybe version of Object::SetPrototype()
010e466 
Fixes the following deprecation warning:

    ../src/node_api.cc:2020:30: warning: 'bool
    v8::Object::SetPrototype(v8::Local<v8::Value>)' is deprecated: Use
    maybe version [-Wdeprecated-declarations]
       wrapper->SetPrototype(proto);
    ../src/node_api.cc:2021:28: warning: 'bool
    v8::Object::SetPrototype(v8::Local<v8::Value>)' is deprecated: Use
    maybe version [-Wdeprecated-declarations]
       obj->SetPrototype(wrapper);

PR-URL: #14053
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
@bnoordhuis @Fishrock123
n-api: fix -Wmaybe-uninitialized compiler warning
dbd629e 
Not an actual bug, as far as I can tell, the compiler is simply not
smart enough to figure out that the offending code path isn't reached
with an uninitialized value.

PR-URL: #14053
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
@bnoordhuis
n-api: use Maybe version of Object::SetPrototype()
c95b561 
Fixes the following deprecation warning:

    ../src/node_api.cc:2020:30: warning: 'bool
    v8::Object::SetPrototype(v8::Local<v8::Value>)' is deprecated: Use
    maybe version [-Wdeprecated-declarations]
       wrapper->SetPrototype(proto);
    ../src/node_api.cc:2021:28: warning: 'bool
    v8::Object::SetPrototype(v8::Local<v8::Value>)' is deprecated: Use
    maybe version [-Wdeprecated-declarations]
       obj->SetPrototype(wrapper);

PR-URL: nodejs#14053
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
@bnoordhuis
n-api: fix -Wmaybe-uninitialized compiler warning
c5c580c 
Not an actual bug, as far as I can tell, the compiler is simply not
smart enough to figure out that the offending code path isn't reached
with an uninitialized value.

PR-URL: nodejs#14053
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@bnoordhuis @MylesBorins
n-api: use Maybe version of Object::SetPrototype()
33821c3 
Fixes the following deprecation warning:

    ../src/node_api.cc:2020:30: warning: 'bool
    v8::Object::SetPrototype(v8::Local<v8::Value>)' is deprecated: Use
    maybe version [-Wdeprecated-declarations]
       wrapper->SetPrototype(proto);
    ../src/node_api.cc:2021:28: warning: 'bool
    v8::Object::SetPrototype(v8::Local<v8::Value>)' is deprecated: Use
    maybe version [-Wdeprecated-declarations]
       obj->SetPrototype(wrapper);

Backport-PR-URL: #19447
PR-URL: #14053
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@bnoordhuis @MylesBorins
n-api: fix -Wmaybe-uninitialized compiler warning
8e2a26d 
Not an actual bug, as far as I can tell, the compiler is simply not
smart enough to figure out that the offending code path isn't reached
with an uninitialized value.

Backport-PR-URL: #19447
PR-URL: #14053
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.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

@fhinkel fhinkel fhinkel approved these changes

@benjamingr benjamingr benjamingr approved these changes

@targos targos targos approved these changes

@cjihrig cjihrig cjihrig approved these changes

+1 more reviewer

@danbev danbev danbev 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.
8 participants
@bnoordhuis @targos @fhinkel @danbev @benjamingr @cjihrig @MylesBorins @nodejs-github-bot