Skip to content

src: remove unnecessary std::endl usage#30003

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
danbev wants to merge 2 commits into from
Closed

src: remove unnecessary std::endl usage #30003

danbev wants to merge 2 commits into from

Conversation

@danbev
Copy link
Contributor

@danbevdanbev commented Oct 17, 2019

This commit removes a few std::endl that could be replaced by '\n' as it
does not look like the buffer needs to be flushed in these places.

This is only done in error handling, and once when the report has been
generated, so this is very minor but I thought I'd bring it up in case
it was overlooked.

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

@danbev
src: remove unnecessary std::endl usage
0254759
This commit removes a few std::endl that could be replaced by '\n' as it does not look like the buffer needs to be flushed in these places. This is only done in error handling, and once when the report has been generated, so this is very minor but I thought I'd bring it up in case it was overlooked.
@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. report Issues and PRs related to process.report. labels Oct 17, 2019
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 17, 2019

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

@gireeshpunathil
Copy link
Member

gireeshpunathil commented Oct 17, 2019•
edited
Loading

@DanDev@danbev - While I agree the buffering at proposed locations do no matter, are there any C++ best practices around usage of one over the other? either in terms of style, convenience or in terms of efficiency?

EDIT: sorry, I guess it went to wrong Daniel.

addaleax
addaleax approved these changes Oct 17, 2019
View reviewed changes
Copy link
Member

@addaleaxaddaleax left a comment

Choose a reason for hiding this comment

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

You can also make the newlines part of the following strings directly, e.g. `"\nNode.js report completed" 🙂

@addaleax
Copy link
Member

addaleax commented Oct 17, 2019

While I agree the buffering at proposed locations do no matter, are there any C++ best practices around usage of one over the other? either in terms of style, convenience or in terms of efficiency?

@gireeshpunathilstd::endl is a combination of \n and std::flush, so it’s only really useful if do you want the output stream to be flushed.

@danbev
Copy link
ContributorAuthor

danbev commented Oct 17, 2019

You can also make the newlines part of the following strings directly, e.g. `"\nNode.js report completed"

😄 I actually did that first and then changed it back as I thought it might be more readable this way and similar to the original code. But I'm happy to change that if no one objects.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 21, 2019

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

@danbev
Copy link
ContributorAuthor

danbev commented Oct 22, 2019•
edited
Loading

Re-run of failing node-test-commit-freebsd ✔️
Re-run of failing node-test-commit-windows-fanned ✔️

@danbevdanbev added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 22, 2019
@Trott
Copy link
Member

Trott commented Oct 22, 2019

Landed in 4187fcb

@TrottTrott closed this Oct 22, 2019
Trott pushed a commit that referenced this pull request Oct 22, 2019
@danbev@Trott
src: remove unnecessary std::endl usage
4187fcb
This commit removes a few std::endl that could be replaced by '\n' as it does not look like the buffer needs to be flushed in these places. This is only done in error handling, and once when the report has been generated, so this is very minor but I thought I'd bring it up in case it was overlooked. PR-URL: #30003 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
@danbevdanbev deleted the node_report_endl branch October 23, 2019 03:58
MylesBorins pushed a commit that referenced this pull request Oct 23, 2019
@danbev@MylesBorins
src: remove unnecessary std::endl usage
84a7e0e
This commit removes a few std::endl that could be replaced by '\n' as it does not look like the buffer needs to be flushed in these places. This is only done in error handling, and once when the report has been generated, so this is very minor but I thought I'd bring it up in case it was overlooked. PR-URL: #30003 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2019
@danbev@MylesBorins
src: remove unnecessary std::endl usage
d561321
This commit removes a few std::endl that could be replaced by '\n' as it does not look like the buffer needs to be flushed in these places. This is only done in error handling, and once when the report has been generated, so this is very minor but I thought I'd bring it up in case it was overlooked. PR-URL: #30003 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Oct 23, 2019
Merged
targos pushed a commit that referenced this pull request Nov 8, 2019
@danbev@targos
src: remove unnecessary std::endl usage
fecc767
This commit removes a few std::endl that could be replaced by '\n' as it does not look like the buffer needs to be flushed in these places. This is only done in error handling, and once when the report has been generated, so this is very minor but I thought I'd bring it up in case it was overlooked. PR-URL: #30003 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
targos pushed a commit that referenced this pull request Nov 10, 2019
@danbev@targos
src: remove unnecessary std::endl usage
f8bfe5a
This commit removes a few std::endl that could be replaced by '\n' as it does not look like the buffer needs to be flushed in these places. This is only done in error handling, and once when the report has been generated, so this is very minor but I thought I'd bring it up in case it was overlooked. PR-URL: #30003 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
@targostargos mentioned this pull request Nov 10, 2019
Merged
targos pushed a commit that referenced this pull request Nov 11, 2019
@danbev@targos
src: remove unnecessary std::endl usage
f6655b4
This commit removes a few std::endl that could be replaced by '\n' as it does not look like the buffer needs to be flushed in these places. This is only done in error handling, and once when the report has been generated, so this is very minor but I thought I'd bring it up in case it was overlooked. PR-URL: #30003 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleaxaddaleaxaddaleax approved these changes

@lpincalpincalpinca approved these changes

@cjihrigcjihrigcjihrig approved these changes

@tniessentniessentniessen approved these changes

@richardlaurichardlaurichardlau approved these changes

@gireeshpunathilgireeshpunathilgireeshpunathil 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.c++Issues and PRs that require attention from people who are familiar with C++.reportIssues and PRs related to process.report.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@danbev@nodejs-github-bot@gireeshpunathil@addaleax@Trott@lpinca@cjihrig@tniessen@richardlau