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

test: fix warning in test_environment.cc #36846

Closed
wants to merge 1 commit into from
Closed

test: fix warning in test_environment.cc #36846

wants to merge 1 commit into from

Conversation

RaisinTen
Copy link
Contributor

warning log:

../test/cctest/test_environment.cc: In constructor ‘RedirectStdErr::RedirectStdErr(const char*)’:
../test/cctest/test_environment.cc:77:12: warning: ignoring return value of ‘FILE* freopen(const char*, const char*, FILE*)’, declared with attribute warn_unused_result [-Wunused-result]
     freopen(filename_, "w", stderr);
     ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 8, 2021
@RaisinTen RaisinTen force-pushed the test/fix-warning-in-test_environment.cc branch from f5706f3 to f67a8bd Compare January 9, 2021 09:02
@RaisinTen
Copy link
Contributor Author

cc @nodejs/testing

@targos
Copy link
Member

targos commented Jan 9, 2021

The USE macro doesn't work in this context? See

node/src/env.cc

Line 467 in a45a404

USE(script->Run(context()));
for example.

@RaisinTen
Copy link
Contributor Author

@targos thanks for mentioning that. I actually tried (void) <the-FILE*> and it still produced the warning. The ! seems to be necessary. I'm not quite sure how I'm supposed to include the header though given that none of the tests use USE. Do I hardcode deps/v8/src/base/macros.h in?

@targos
Copy link
Member

targos commented Jan 9, 2021

Sorry, it's not a macro for us. The function is defined in src/util.h. You can use it like this: node::USE(...)

@RaisinTen
Copy link
Contributor Author

@targos thanks, now the warning is gone. :)

@RaisinTen
test: fix warning in test_environment.cc
3e7ef4b 
warning log:
../test/cctest/test_environment.cc: In constructor ‘RedirectStdErr::RedirectStdErr(const char*)’:
../test/cctest/test_environment.cc:77:12: warning: ignoring return value of ‘FILE* freopen(const char*, const char*, FILE*)’, declared with attribute warn_unused_result [-Wunused-result]
     freopen(filename_, "w", stderr);
     ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
@RaisinTen RaisinTen force-pushed the test/fix-warning-in-test_environment.cc branch from 9ae438f to 3e7ef4b Compare January 13, 2021 15:01
@RaisinTen RaisinTen added the review wanted PRs that need reviews. label Jan 17, 2021
aduh95
aduh95 approved these changes Jan 18, 2021
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

RSLGTM

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 18, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@jasnell
Copy link
Member

jasnell commented Jan 18, 2021

Landed in 341bbd3

@jasnell jasnell closed this Jan 18, 2021
jasnell pushed a commit that referenced this pull request Jan 18, 2021
@RaisinTen @jasnell
test: fix warning in test_environment.cc
341bbd3 
```
warning log:
../test/cctest/test_environment.cc: In constructor   \
‘RedirectStdErr::RedirectStdErr(const char*)’:
../test/cctest/test_environment.cc:77:12: warning:   \
ignoring return value of ‘FILE* freopen(const char*, \
const char*, FILE*)’, declared with attribute
warn_unused_result [-Wunused-result]
     freopen(filename_, "w", stderr);
     ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
```

PR-URL: #36846
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RaisinTen RaisinTen deleted the test/fix-warning-in-test_environment.cc branch January 19, 2021 13:49
@ruyadorno ruyadorno removed the review wanted PRs that need reviews. label Jan 21, 2021
ruyadorno pushed a commit that referenced this pull request Jan 22, 2021
@RaisinTen @ruyadorno
test: fix warning in test_environment.cc
3d20415 
```
warning log:
../test/cctest/test_environment.cc: In constructor   \
‘RedirectStdErr::RedirectStdErr(const char*)’:
../test/cctest/test_environment.cc:77:12: warning:   \
ignoring return value of ‘FILE* freopen(const char*, \
const char*, FILE*)’, declared with attribute
warn_unused_result [-Wunused-result]
     freopen(filename_, "w", stderr);
     ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
```

PR-URL: #36846
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Jan 22, 2021
Merged
ruyadorno pushed a commit that referenced this pull request Jan 25, 2021
@RaisinTen @ruyadorno
test: fix warning in test_environment.cc
8d49ce9 
```
warning log:
../test/cctest/test_environment.cc: In constructor   \
‘RedirectStdErr::RedirectStdErr(const char*)’:
../test/cctest/test_environment.cc:77:12: warning:   \
ignoring return value of ‘FILE* freopen(const char*, \
const char*, FILE*)’, declared with attribute
warn_unused_result [-Wunused-result]
     freopen(filename_, "w", stderr);
     ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
```

PR-URL: #36846
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request May 1, 2021
@RaisinTen @targos
test: fix warning in test_environment.cc
90122d8 
```
warning log:
../test/cctest/test_environment.cc: In constructor   \
‘RedirectStdErr::RedirectStdErr(const char*)’:
../test/cctest/test_environment.cc:77:12: warning:   \
ignoring return value of ‘FILE* freopen(const char*, \
const char*, FILE*)’, declared with attribute
warn_unused_result [-Wunused-result]
     freopen(filename_, "w", stderr);
     ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
```

PR-URL: #36846
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 approved these changes

@jasnell jasnell jasnell 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. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@RaisinTen @targos @nodejs-github-bot @jasnell @aduh95 @ruyadorno