Skip to content

test: fix warning in test_environment.cc#36846

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
Closed
RaisinTen wants to merge 1 commit into from
Closed

test: fix warning in test_environment.cc #36846

RaisinTen wants to merge 1 commit into from

Conversation

@RaisinTen
Copy link
Member

@RaisinTenRaisinTen commented Jan 8, 2021

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-botnodejs-github-bot added the test Issues and PRs related to the tests. label Jan 8, 2021
@RaisinTenRaisinTenforce-pushed the test/fix-warning-in-test_environment.cc branch from f5706f3 to f67a8bdCompareJanuary 9, 2021 09:02
@RaisinTen
Copy link
MemberAuthor

RaisinTen commented Jan 9, 2021

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
MemberAuthor

RaisinTen commented Jan 9, 2021

@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
MemberAuthor

RaisinTen commented Jan 9, 2021

@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); ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
@RaisinTenRaisinTenforce-pushed the test/fix-warning-in-test_environment.cc branch from 9ae438f to 3e7ef4bCompareJanuary 13, 2021 15:01
@RaisinTenRaisinTen 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

@aduh95aduh95 left a comment

Choose a reason for hiding this comment

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

RSLGTM

@aduh95aduh95 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

nodejs-github-bot commented Jan 18, 2021

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

@jasnell
Copy link
Member

jasnell commented Jan 18, 2021

Landed in 341bbd3

@jasnelljasnell 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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
@RaisinTenRaisinTen deleted the test/fix-warning-in-test_environment.cc branch January 19, 2021 13:49
@ruyadornoruyadorno 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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
@ruyadornoruyadorno 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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
@danielleadamsdanielleadams mentioned this pull request May 3, 2021
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes

@aduh95aduh95aduh95 approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.testIssues 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