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: fix warn_unused_result compiler warning #32241

Merged
merged 1 commit into from
Mar 15, 2020
Merged

src: fix warn_unused_result compiler warning #32241

merged 1 commit into from
Mar 15, 2020

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Mar 13, 2020 

../src/node_file.cc:386:7: warning: ignoring return value of function declared with 'warn_unused_result' attribute [-Wunused-result]

Refs: #31972

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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. labels Mar 13, 2020
@sam-github
Copy link
Contributor

I wonder if we could add tests for this kind of thing. Maybe we are running into a gyp limitation, -Werror doesn't work for us, because the deps spew warnings. Is it possible to cause just the src/ files, node's src, to fail on warning? Or... maybe that won't work because of v8 warnings.

addaleax
addaleax requested changes Mar 13, 2020
View reviewed changes
src/node_file.cc Outdated Show resolved Hide resolved
@addaleax
Copy link
Member

@sam-github One problem with -Werror for the src/ tree might be that it could fail on some compiler versions and not others :/

@sam-github
Copy link
Contributor

It could, but then we would see, and fix it, as opposed to people later noticing and there being a stream (ok, continuous trickle?) of PRs to fix the warnings. I don't think we've a situation where there is a warning that can't be fixed across gcc* and clang*. I have in the past worked on multi-platform code where it wasn't possible to make code that satisfied all our compilers simultaneously, but I think clang and gcc are under some kind of social or technical pressure to not diverge like that.

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 13, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 13, 2020 •
edited by cjihrig
Loading

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

@cjihrig
src: fix warn_unused_result compiler warning
8617508 
../src/node_file.cc:386:7: warning: ignoring return value of
function declared with 'warn_unused_result'
attribute [-Wunused-result]

PR-URL: nodejs#32241
Refs: nodejs#31972
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@cjihrig cjihrig merged commit 8617508 into nodejs:master Mar 15, 2020
@cjihrig cjihrig deleted the warning branch March 15, 2020 13:51
@sthagen sthagen mentioned this pull request Mar 15, 2020
Merged
4 tasks
BridgeAR pushed a commit that referenced this pull request Mar 17, 2020
@cjihrig @BridgeAR
src: fix warn_unused_result compiler warning
3c0ce69 
../src/node_file.cc:386:7: warning: ignoring return value of
function declared with 'warn_unused_result'
attribute [-Wunused-result]

PR-URL: #32241
Refs: #31972
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 19, 2020
Merged
MylesBorins pushed a commit that referenced this pull request Mar 24, 2020
@cjihrig @MylesBorins
src: fix warn_unused_result compiler warning
7fc5e6d 
../src/node_file.cc:386:7: warning: ignoring return value of
function declared with 'warn_unused_result'
attribute [-Wunused-result]

PR-URL: #32241
Refs: #31972
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 22, 2020
@cjihrig @targos
src: fix warn_unused_result compiler warning
9b6f678 
../src/node_file.cc:386:7: warning: ignoring return value of
function declared with 'warn_unused_result'
attribute [-Wunused-result]

PR-URL: #32241
Refs: #31972
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Apr 22, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devnexen devnexen devnexen approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@sam-github sam-github sam-github approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cjihrig @sam-github @addaleax @nodejs-github-bot @jasnell @devnexen