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

 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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

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

Uh oh!

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

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

Uh oh!

Conversation

danbev commented May 24, 2017

Checklist

Affected core subsystem(s)

Uh oh!

danbev commented May 24, 2017

Uh oh!

refackMay 24, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-githubMay 24, 2017

Choose a reason for hiding this comment

Uh oh!

refackMay 24, 2017

Choose a reason for hiding this comment

Uh oh!

refackMay 24, 2017

Choose a reason for hiding this comment

Uh oh!

sam-githubMay 24, 2017

Choose a reason for hiding this comment

Uh oh!

refackMay 24, 2017

Choose a reason for hiding this comment

Uh oh!

sam-githubMay 24, 2017

Choose a reason for hiding this comment

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

danbev commented May 26, 2017

Uh oh!

addaleax commented May 26, 2017

Uh oh!

addaleax commented May 26, 2017

Uh oh!

danbev commented May 27, 2017

Uh oh!

MylesBorins commented Jul 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

refackMay 24, 2017•
edited
Loading