src: ignore unused warning for inspector-agent.cc #13188

danbev · 2017-05-24T07:48:43Z

Currently the following compiler warning is displayed:

../src/inspector_agent.cc:218:5: warning: ignoring return value of
function declared with warn_unused_result attribute [-Wunused-result]
    callback->Call(env_->context(), receiver, 1, &argument);
    ^~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

This commit does a static cast of the result as there are tests that
fail if we try to do something like ToLocalChecked.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

src

Currently the following compiler warning is displayed: ../src/inspector_agent.cc:218:5: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result] callback->Call(env_->context(), receiver, 1, &argument); ^~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated. This commit does a static cast of the result as there are tests that fail if we try to do something like ToLocalChecked.

danbev · 2017-05-24T07:49:28Z

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

refack · 2017-05-24T13:17:33Z

src/inspector_agent.cc

@@ -215,7 +215,7 @@ class JsBindingsSessionDelegate : public InspectorSessionDelegate {
    Local<Value> argument = v8string.ToLocalChecked().As<Value>();
    Local<Function> callback = callback_.Get(isolate);
    Local<Object> receiver = receiver_.Get(isolate);
-    callback->Call(env_->context(), receiver, 1, &argument);
+    static_cast<void>(callback->Call(env_->context(), receiver, 1, &argument));


~~Wouldn't~~

#pragma GCC unused-result push #pragma GCC unused-result ignored callback->Call(env_->context(), receiver, 1, &argument) #pragma GCC unused-result pop

~~be better, so it could be traced in the future~~

~~Or even just a comment stating the the // static_cast<void> is to skip 'unused-result' warning?~~

Dismissed

No comment on the portability of the pragma, but there is no reason to cast to void other than to avoid warnings and visually document that the return value is explicitly being ignored, I don't think a comment is needed for that usage.

Agreed if we document this somewhere as an idiom static_cast<void> === #pragma GCC unused-result ignored
Because a quick search only found one other use, in this file in line 210@master 352@here.

Since we don't have a style guide, I'll add a note to #12791

Casting to void is standard C++, https://stackoverflow.com/questions/689677/why-cast-unused-return-values-to-void, unlike GCC pramas.

I already agreed 👌

Sorry, I thought you wanted this documented as a node-specific style. Nice emoji use.

cjihrig

LGTM, but coverity can also be suppressed using a comment (search src/ for coverity).

mhdawson

LGTM

danbev · 2017-05-26T03:44:54Z

LGTM, but coverity can also be suppressed using a comment (search src/ for coverity).

Sorry if this is documented somewhere but I must have missed it, is there a way to run coverity or get to the actual reports an other way then at this page?

I would have thought that any warning from coverity would go away with the casting suggested in this pull request.

addaleax · 2017-05-26T19:10:06Z

I would have thought that any warning from coverity would go away with the casting suggested in this pull request.

I think @cjihrig was implying that adding the comment would be an alternative approach to casting to void (but I agree that casting is better than just to explicitly disable the warning for a single tool), not that this PR doesn’t suffice.

addaleax · 2017-05-26T19:10:35Z

Landed in 2db6556

Currently the following compiler warning is displayed: ../src/inspector_agent.cc:218:5: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result] callback->Call(env_->context(), receiver, 1, &argument); ^~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated. This commit does a static cast of the result as there are tests that fail if we try to do something like ToLocalChecked. PR-URL: #13188 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

danbev · 2017-05-27T03:57:24Z

I think @cjihrig was implying that adding the comment would be an alternative approach to casting to void (but I agree that casting is better than just to explicitly disable the warning for a single tool), not that this PR doesn’t suffice.

Ah I got it now. Thanks!

Currently the following compiler warning is displayed: ../src/inspector_agent.cc:218:5: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result] callback->Call(env_->context(), receiver, 1, &argument); ^~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated. This commit does a static cast of the result as there are tests that fail if we try to do something like ToLocalChecked. PR-URL: #13188 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins · 2017-07-17T17:01:27Z

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace do-not-land if it is being backported

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x inspector Issues and PRs related to the V8 inspector protocol labels May 24, 2017

bnoordhuis approved these changes May 24, 2017

View reviewed changes

jasnell approved these changes May 24, 2017

View reviewed changes

refack approved these changes May 24, 2017

View reviewed changes

refack reviewed May 24, 2017

View reviewed changes

cjihrig approved these changes May 24, 2017

View reviewed changes

mhdawson approved these changes May 24, 2017

View reviewed changes

addaleax approved these changes May 24, 2017

View reviewed changes

addaleax closed this May 26, 2017

jasnell mentioned this pull request May 28, 2017

8.0.0 Release Proposal #12220

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

danbev deleted the ignore-unused-result-warning-inspector-agent branch June 28, 2017 05:36

MylesBorins added the dont-land-on-v6.x label Jul 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: ignore unused warning for inspector-agent.cc #13188

src: ignore unused warning for inspector-agent.cc #13188

danbev commented May 24, 2017

danbev commented May 24, 2017

refack May 24, 2017 •

edited

Loading

sam-github May 24, 2017

refack May 24, 2017

refack May 24, 2017

sam-github May 24, 2017

refack May 24, 2017

sam-github May 24, 2017

cjihrig left a comment

mhdawson left a comment

danbev commented May 26, 2017

addaleax commented May 26, 2017

addaleax commented May 26, 2017

danbev commented May 27, 2017

MylesBorins commented Jul 17, 2017

src: ignore unused warning for inspector-agent.cc #13188

src: ignore unused warning for inspector-agent.cc #13188

Conversation

danbev commented May 24, 2017

Checklist

Affected core subsystem(s)

danbev commented May 24, 2017

refack May 24, 2017 • edited Loading

Choose a reason for hiding this comment

sam-github May 24, 2017

Choose a reason for hiding this comment

refack May 24, 2017

Choose a reason for hiding this comment

refack May 24, 2017

Choose a reason for hiding this comment

sam-github May 24, 2017

Choose a reason for hiding this comment

refack May 24, 2017

Choose a reason for hiding this comment

sam-github May 24, 2017

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

danbev commented May 26, 2017

addaleax commented May 26, 2017

addaleax commented May 26, 2017

danbev commented May 27, 2017

MylesBorins commented Jul 17, 2017

refack May 24, 2017 •

edited

Loading