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: remove unnecessary std::endl usage #30003

Closed
wants to merge 2 commits into from
Closed

src: remove unnecessary std::endl usage #30003

wants to merge 2 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev 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-bot nodejs-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

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

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

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?

@gireeshpunathil std::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
Contributor Author

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

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

@danbev
Copy link
Contributor Author

danbev commented Oct 22, 2019 •
edited
Loading

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

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

@Trott Trott 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]>
@danbev danbev 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]>
@MylesBorins MylesBorins 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]>
@targos targos 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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@richardlau richardlau richardlau approved these changes

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax 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++. report Issues 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