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

benchmark,lib,test: use braces for multiline block #13828

Closed
wants to merge 1 commit into from
Closed

benchmark,lib,test: use braces for multiline block #13828

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jun 20, 2017

For if/else and loops where the bodies span more than one line, use
curly braces.

Refs: #13623 (comment)

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

benchmark lib test

@Trott
benchmark,lib,test: use braces for multiline block
0ecba24 
For if/else and loops where the bodies span more than one line, use
curly braces.

Refs: nodejs#13623 (comment)
@nodejs-github-bot nodejs-github-bot added inspector Issues and PRs related to the V8 inspector protocol lib / src Issues and PRs related to general changes in the lib or src directory. labels Jun 20, 2017
@Trott
Copy link
Member Author

Trott commented Jun 20, 2017

/cc @addaleax

@addaleax addaleax removed the inspector Issues and PRs related to the V8 inspector protocol label Jun 20, 2017
mscdex
mscdex reviewed Jun 20, 2017
View reviewed changes
benchmark/buffers/buffer-iterate.js
@@ -29,20 +29,23 @@ function main(conf) {
function benchFor(buffer, n) {
bench.start();

for (var k = 0; k < n; k++)
for (var i = 0; i < buffer.length; i++)
for (var k = 0; k < n; k++) {
Copy link
Contributor

@mscdex mscdex Jun 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I get why braces would be added here, but why also on the for below if it does not contain a multi-line body? Ditto for all other instances of this in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mscdex To make the decision about things like that, I looked at surrounding code and also used my own judgment as to what was more readable.

In this case, having one for with braces wrapping another for without braces seemed less readable to me than both having braces. Additionally, there were no other examples in this file of blocks without braces.

@Trott
Copy link
Member Author

Trott commented Jun 23, 2017

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

@refack
Copy link
Contributor

refack commented Jun 23, 2017

After such changes I usually ask: "Can you now turn on an ESLint rule?"

Trott added a commit to Trott/io.js that referenced this pull request Jun 23, 2017
@Trott
benchmark,lib,test: use braces for multiline block
095c0de 
For if/else and loops where the bodies span more than one line, use
curly braces.

PR-URL: nodejs#13828
Ref: nodejs#13623 (comment)
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented Jun 23, 2017

Landed in 095c0de

@Trott Trott closed this Jun 23, 2017
@addaleax
Copy link
Member

@Trott This doesn’t land cleanly on 8.x; if you can backport it, great, but if you don’t think it’s worth it feel free to add the dont-land label.

Trott added a commit to Trott/io.js that referenced this pull request Jun 29, 2017
@Trott
benchmark,lib,test: use braces for multiline block
a975b8d 
For if/else and loops where the bodies span more than one line, use
curly braces.

PR-URL: nodejs#13828
Ref: nodejs#13623 (comment)
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented Jun 30, 2017

@addaleax backport at #13995

addaleax pushed a commit that referenced this pull request Jun 30, 2017
@Trott @addaleax
benchmark,lib,test: use braces for multiline block
8152383 
For if/else and loops where the bodies span more than one line, use
curly braces.

Original-PR-URL: #13828
Ref: #13623 (comment)
Original-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Original-Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Original-Reviewed-By: Michaël Zasso <targos@protonmail.com>
Original-Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #13995
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request Jun 30, 2017
@Trott @addaleax
benchmark,lib,test: use braces for multiline block
70a7919 
For if/else and loops where the bodies span more than one line, use
curly braces.

Original-PR-URL: #13828
Ref: #13623 (comment)
Original-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Original-Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Original-Reviewed-By: Michaël Zasso <targos@protonmail.com>
Original-Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #13995
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
@Trott @addaleax
benchmark,lib,test: use braces for multiline block
62f8f0f 
For if/else and loops where the bodies span more than one line, use
curly braces.

Original-PR-URL: #13828
Ref: #13623 (comment)
Original-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Original-Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Original-Reviewed-By: Michaël Zasso <targos@protonmail.com>
Original-Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #13995
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@Trott @addaleax
benchmark,lib,test: use braces for multiline block
3d0b66a 
For if/else and loops where the bodies span more than one line, use
curly braces.

Original-PR-URL: #13828
Ref: #13623 (comment)
Original-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Original-Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Original-Reviewed-By: Michaël Zasso <targos@protonmail.com>
Original-Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #13995
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins
Copy link
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
@Trott Trott deleted the tmp branch January 13, 2022 22:45
@GeoffreyBooth GeoffreyBooth mentioned this pull request May 2, 2022
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@targos targos targos approved these changes

@cjihrig cjihrig cjihrig approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@Trott @refack @addaleax @MylesBorins @mscdex @jasnell @targos @cjihrig @BridgeAR @nodejs-github-bot