Skip to content

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Feb 18, 2020

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@devsnek devsnek requested a review from bnoordhuis February 18, 2020 10:22
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 18, 2020
@devsnek devsnek force-pushed the refactor-internal-readfile branch from 1a534df to dd0ef45 Compare February 18, 2020 10:23
@devsnek devsnek removed the lib / src Issues and PRs related to general changes in the lib or src directory. label Feb 18, 2020
@devsnek devsnek force-pushed the refactor-internal-readfile branch 2 times, most recently from 7b89da3 to 6f017b7 Compare February 18, 2020 19:45
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM but can you drop the braces around the single-statement if statements?

@devsnek devsnek force-pushed the refactor-internal-readfile branch from 6f017b7 to 4d16408 Compare February 19, 2020 16:59
@devsnek
Copy link
Member Author

devsnek commented Feb 19, 2020

@bnoordhuis i'd rather not do braceless if it isn't too much of a problem.

@bnoordhuis
Copy link
Member

It's at odds with the house style. Let me turn it around: what's the problem with braceless style?

@devsnek
Copy link
Member Author

devsnek commented Feb 19, 2020

@bnoordhuis i don't think we really have a style one way or the other (if we wanted to, we should add a lint). The problem i have with braceless is that if braces eventually need to be added for multiple statements it shows an extra two lines of change in the diff.

@bnoordhuis
Copy link
Member

Majority rule. I did a quick scan - <insert not exact science warning> - and the braceless variant makes up 60% of the single-line if statements:

# braces
$ find src -name \*.cc | xargs cat | perl -e 'while(<>){s/\s+//g;$i=$.+2 if/^if\(.+\{/;$n++ if/^\}/&&$.==$i}print"$n\n"'
582

# braceless
$ find src -name \*.cc | xargs cat | grep -E 'if \(.+\)' | grep -v '{' | grep -c .
1419

The if statements with braces usually have multi-line else clauses.

@devsnek
Copy link
Member Author

devsnek commented Feb 20, 2020

is there some specific problem you have with braces? It seems they're prevalent in our source, and we don't have a lint against them, so I'm not sure why I wouldn't be able to use them.

@bnoordhuis
Copy link
Member

The prevailing style is to omit braces, ergo, new code should omit braces. Consistency trumps personal preference.

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
@jasnell
Copy link
Member

jasnell commented Jun 25, 2020

Ping @devsnek .. needs to be rebased

@aduh95 aduh95 added the stalled Issues and PRs that are stalled. label Nov 8, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2020

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

@devsnek devsnek closed this Dec 24, 2020
@devsnek devsnek deleted the refactor-internal-readfile branch December 24, 2020 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants