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.

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

src: remove unnecessary std::endl usage #30003

Closed
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

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

@gireeshpunathil
Copy link
Member

gireeshpunathil commented Oct 17, 2019

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

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

@danbev
Copy link
Contributor Author

danbev commented Oct 22, 2019

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
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 <gpunathi@in.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@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
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 <gpunathi@in.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this pull request Oct 23, 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.

PR-URL: #30003
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@MylesBorins MylesBorins mentioned this pull request Oct 23, 2019
targos pushed a commit that referenced this pull request Nov 8, 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.

PR-URL: #30003
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
targos pushed a commit that referenced this pull request Nov 10, 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.

PR-URL: #30003
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
targos pushed a commit that referenced this pull request Nov 11, 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.

PR-URL: #30003
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

9 participants