Skip to content
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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented May 24, 2017

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

@danbev
src: ignore unused warning for inspector-agent.cc
7ffbbeb 
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.
@nodejs-github-bot 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
@danbev
Copy link
Contributor Author

danbev commented May 24, 2017

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

refack
refack reviewed May 24, 2017
View reviewed changes
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));
Copy link
Contributor

@refack refack May 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I already agreed 👌

Copy link
Contributor

Choose a reason for hiding this comment

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

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

cjihrig
cjihrig approved these changes May 24, 2017
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

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

mhdawson
mhdawson approved these changes May 24, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@danbev
Copy link
Contributor Author

danbev commented May 26, 2017

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

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

Landed in 2db6556

@addaleax addaleax closed this May 26, 2017
addaleax pushed a commit that referenced this pull request May 26, 2017
@danbev @addaleax
src: ignore unused warning for inspector-agent.cc
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
Copy link
Contributor Author

danbev commented May 27, 2017

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!

jasnell pushed a commit that referenced this pull request May 28, 2017
@danbev @jasnell
src: ignore unused warning for inspector-agent.cc
e7d098c 
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]>
@jasnell jasnell mentioned this pull request May 28, 2017
Closed
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
@danbev danbev deleted the ignore-unused-result-warning-inspector-agent branch June 28, 2017 05:36
@MylesBorins
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sam-github sam-github sam-github left review comments

@refack refack refack approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@mhdawson mhdawson mhdawson approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@danbev @addaleax @MylesBorins @sam-github @refack @bnoordhuis @jasnell @cjihrig @mhdawson @nodejs-github-bot