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

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

Closed
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

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

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

@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
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member Author

Trott commented Jun 23, 2017

Landed in 095c0de

@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
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@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
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 <[email protected]>
Original-Reviewed-By: Colin Ihrig <[email protected]>
Original-Reviewed-By: Michaël Zasso <[email protected]>
Original-Reviewed-By: James M Snell <[email protected]>

PR-URL: #13995
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Jun 30, 2017
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 <[email protected]>
Original-Reviewed-By: Colin Ihrig <[email protected]>
Original-Reviewed-By: Michaël Zasso <[email protected]>
Original-Reviewed-By: James M Snell <[email protected]>

PR-URL: #13995
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
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 <[email protected]>
Original-Reviewed-By: Colin Ihrig <[email protected]>
Original-Reviewed-By: Michaël Zasso <[email protected]>
Original-Reviewed-By: James M Snell <[email protected]>

PR-URL: #13995
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
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 <[email protected]>
Original-Reviewed-By: Colin Ihrig <[email protected]>
Original-Reviewed-By: Michaël Zasso <[email protected]>
Original-Reviewed-By: James M Snell <[email protected]>

PR-URL: #13995
Reviewed-By: Anna Henningsen <[email protected]>
@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants