Skip to content

Conversation

@bnoordhuis
Copy link
Member

@bnoordhuisbnoordhuis commented Jul 3, 2017

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-botnodejs-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
Copy link
Member

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

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

@bnoordhuis
Copy link
MemberAuthor

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.

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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
@bnoordhuisbnoordhuis deleted the fix-napi-warnings branch July 5, 2017 14:54
@bnoordhuisbnoordhuis merged commit e36917b into nodejs:masterJul 5, 2017
addaleax pushed a commit that referenced this pull request Jul 11, 2017
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
@addaleaxaddaleax mentioned this pull request Jul 11, 2017
addaleax pushed a commit that referenced this pull request Jul 18, 2017
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Apr 16, 2018
Sign up for freeto 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-apiIssues and PRs related to the Node-API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@bnoordhuis@targos@fhinkel@danbev@benjamingr@cjihrig@MylesBorins@nodejs-github-bot