Hi all-
A few weeks ago I noticed that our "omit braces with single line
blocks" rule wasn't written down! Additionally, as a group on IRC and
in review, noticed that the enforcement of this rule has been extremely
inconsistent. We made a first run at codifying our existing practice here:
https://reviews.llvm.org/D80947, which was then committed after significant time
on llvm-commits.
I would like to encourage the list via discussion and further reviews/commits to
come to a consensus on what we actually MEAN by this rule. For example, a
recent comment points out that :
If (cond)
Stmt;
else if (cond)
Stmt;
else {
Stmt;
Stmt;
}
Should require braces on all of the conditions! However, we are extraordinarily
inconsistent here. My wish is for us to become more consistent, so I would like
us to use this thread to organize our collective thoughts on figuring out what
the rule actually SHOULD be, and organizing a handful of commits to the coding
standard to make sure it says what we mean.
Thanks,
Erich
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200615/8508aa9c/attachment.html>
> On Jun 15, 2020, at 15:46, Keane, Erich via llvm-dev <llvm-dev at lists.llvm.org> wrote: > > Hi all- > > A few weeks ago I noticed that our “omit braces with single line blocks” rule wasn’t written down! Additionally, as a group on IRC and in review, noticed that the enforcement of this rule has been extremely inconsistent. We made a first run at codifying our existing practice here: https://reviews.llvm.org/D80947 <https://reviews.llvm.org/D80947>, which was then committed after significant time on llvm-commits. > > I would like to encourage the list via discussion and further reviews/commits to come to a consensus on what we actually MEAN by this rule. For example, a recent comment points out that : > > If (cond) > Stmt; > else if (cond) > Stmt; > else { > Stmt; > Stmt; > } > > Should require braces on all of the conditions! However, we are extraordinarily inconsistent here. My wish is for us to become more consistent, so I would like us to use this thread to organize our collective thoughts on figuring out what the rule actually SHOULD be, and organizing a handful of commits to the coding standard to make sure it says what we mean. > > Thanks, > ErichI think braces should be added in all contexts, and the more contexts the better. It eliminates any inconsistency or attempt to contextually interpret rules. It also reduces merge conflicts, since something eventually something will probably be added inside any control flow statement. I’ve suffered through many painful merges trying to find where the braces went wrong, usually due to switch statements. The sometimes-braces-sometimes-not combined with the lack of indentation for switch cases leads to way more time figuring out braces than should be necessary. -Matt -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200615/3018cb00/attachment.html>
For my part, I think I've tended to leave this case as pretty much entirely discretionary - don't think I've ever given code review feedback about adding/removing braces on these associated blocks because there were necessary braces on one of them. (similarly, I don't think I've complained either way about braces on multiline single statement blocks) If I had to pick a rule (not sure we have to) - yeah, I'd probably lean towards the all/nothing camp for associated blocks (& probably /not/ putting braces on multiline single statement blocks). But I think they're both areas where discretion/local readability isn't super problematic. On Mon, Jun 15, 2020 at 12:46 PM Keane, Erich via llvm-dev <llvm-dev at lists.llvm.org> wrote:> > Hi all- > > > > A few weeks ago I noticed that our “omit braces with single line blocks” rule wasn’t written down! Additionally, as a group on IRC and in review, noticed that the enforcement of this rule has been extremely inconsistent. We made a first run at codifying our existing practice here: https://reviews.llvm.org/D80947, which was then committed after significant time on llvm-commits. > > > > I would like to encourage the list via discussion and further reviews/commits to come to a consensus on what we actually MEAN by this rule. For example, a recent comment points out that : > > > > If (cond) > > Stmt; > > else if (cond) > > Stmt; > > else { > > Stmt; > > Stmt; > > } > > > > Should require braces on all of the conditions! However, we are extraordinarily inconsistent here. My wish is for us to become more consistent, so I would like us to use this thread to organize our collective thoughts on figuring out what the rule actually SHOULD be, and organizing a handful of commits to the coding standard to make sure it says what we mean. > > > Thanks, > Erich > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Matt Arsenault via llvm-dev <llvm-dev at lists.llvm.org> writes:> I think braces should be added in all contexts, and the more contexts > the better. It eliminates any inconsistency or attempt to contextually > interpret rules. It also reduces merge conflicts, since something > eventually something will probably be added inside any control flow > statement. I’ve suffered through many painful merges trying to find > where the braces went wrong, usually due to switch statements. The > sometimes-braces-sometimes-not combined with the lack of indentation > for switch cases leads to way more time figuring out braces than > should be necessary.+1. In addition, the lack of braces can lead to subtle bugs if one adds a line to an existing single-line conditional statement. Best to be safe and always use braces. -David
As another data point, the MLIR part of the codebase is pretty consistent on this: never use braces for trivial (single statement) if/else/for, but always put it on every branch if needed on any side of the if/else. We also have clang-format pretty heavily enforced (including in the automated pre-merge testing on phabricator) which does not lead to issues where someone would add something into the body of a `for` for example and "forget" to add braces. I don't think I have seen any single instance of such bugs slipping in our code so far. On Mon, Jun 15, 2020 at 12:46 PM Keane, Erich via llvm-dev < llvm-dev at lists.llvm.org> wrote:> Hi all- > > > > A few weeks ago I noticed that our “omit braces with single line blocks” > rule wasn’t written down! Additionally, as a group on IRC and in review, > noticed that the enforcement of this rule has been extremely inconsistent. > We made a first run at codifying our existing practice here: > https://reviews.llvm.org/D80947, which was then committed after > significant time on llvm-commits. > > > > I would like to encourage the list via discussion and further > reviews/commits to come to a consensus on what we actually MEAN by this > rule. For example, a recent comment points out that : > > > > If (cond) > > Stmt; > > else if (cond) > > Stmt; > > else { > > Stmt; > > Stmt; > > } > > > > Should require braces on all of the conditions! However, we are > extraordinarily inconsistent here. My wish is for us to become more > consistent, so I would like us to use this thread to organize our > collective thoughts on figuring out what the rule actually SHOULD be, and > organizing a handful of commits to the coding standard to make sure it says > what we mean. > > > Thanks, > Erich > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200615/1217beda/attachment.html>
On Mon, Jun 15, 2020 at 4:05 PM Mehdi AMINI via llvm-dev <llvm-dev at lists.llvm.org> wrote:> > As another data point, the MLIR part of the codebase is pretty consistent on this: never use braces for trivial (single statement) if/else/for, but always put it on every branch if needed on any side of the if/else.Any opinion/stance policy/practice on the "one line, or one statement (& possibly comments, etc)" issue?> We also have clang-format pretty heavily enforced (including in the automated pre-merge testing on phabricator) which does not lead to issues where someone would add something into the body of a `for` for example and "forget" to add braces. I don't think I have seen any single instance of such bugs slipping in our code so far. > > > > On Mon, Jun 15, 2020 at 12:46 PM Keane, Erich via llvm-dev <llvm-dev at lists.llvm.org> wrote: >> >> Hi all- >> >> >> >> A few weeks ago I noticed that our “omit braces with single line blocks” rule wasn’t written down! Additionally, as a group on IRC and in review, noticed that the enforcement of this rule has been extremely inconsistent. We made a first run at codifying our existing practice here: https://reviews.llvm.org/D80947, which was then committed after significant time on llvm-commits. >> >> >> >> I would like to encourage the list via discussion and further reviews/commits to come to a consensus on what we actually MEAN by this rule. For example, a recent comment points out that : >> >> >> >> If (cond) >> >> Stmt; >> >> else if (cond) >> >> Stmt; >> >> else { >> >> Stmt; >> >> Stmt; >> >> } >> >> >> >> Should require braces on all of the conditions! However, we are extraordinarily inconsistent here. My wish is for us to become more consistent, so I would like us to use this thread to organize our collective thoughts on figuring out what the rule actually SHOULD be, and organizing a handful of commits to the coding standard to make sure it says what we mean. >> >> >> Thanks, >> Erich >> >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
I'm with Matt on this one. I much prefer the approach of ALWAYS use braces for ifs and for loops, even if they're not needed, for basically the same reasons as he put. The number of times I've added a statement inside an if without braces and forget to add them is annoyingly high, especially as it's not always an obvious error upfront. Similarly, being involved in a downstream codebase with several private patches, having to sometimes add the braces makes merges all the harder and sometimes more dangerous. I doubt we're going to get the policy changed from "don't include unnecessary braces for trivial statements" but if there's any style that adds them in more places, I'm up for that. James On Mon, 15 Jun 2020 at 20:56, Matt Arsenault via llvm-dev < llvm-dev at lists.llvm.org> wrote:> > > On Jun 15, 2020, at 15:46, Keane, Erich via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > > Hi all- > > A few weeks ago I noticed that our “omit braces with single line blocks” > rule wasn’t written down! Additionally, as a group on IRC and in review, > noticed that the enforcement of this rule has been extremely inconsistent. > We made a first run at codifying our existing practice here: > https://reviews.llvm.org/D80947, which was then committed after > significant time on llvm-commits. > > I would like to encourage the list via discussion and further > reviews/commits to come to a consensus on what we actually MEAN by this > rule. For example, a recent comment points out that : > > If (cond) > Stmt; > else if (cond) > Stmt; > else { > Stmt; > Stmt; > } > > Should require braces on all of the conditions! However, we are > extraordinarily inconsistent here. My wish is for us to become more > consistent, so I would like us to use this thread to organize our > collective thoughts on figuring out what the rule actually SHOULD be, and > organizing a handful of commits to the coding standard to make sure it says > what we mean. > > Thanks, > Erich > > > > I think braces should be added in all contexts, and the more contexts the > better. It eliminates any inconsistency or attempt to contextually > interpret rules. It also reduces merge conflicts, since something > eventually something will probably be added inside any control flow > statement. I’ve suffered through many painful merges trying to find where > the braces went wrong, usually due to switch statements. The > sometimes-braces-sometimes-not combined with the lack of indentation for > switch cases leads to way more time figuring out braces than should be > necessary. > > -Matt > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200616/463a54ad/attachment.html>
On 15/06/2020 20:46, Keane, Erich via llvm-dev wrote:> Hi all- > > > > A few weeks ago I noticed that our “omit braces with single line blocks” rule wasn’t written down! Additionally, as a group on IRC and in review, noticed that the enforcement of this rule has been extremely inconsistent. We made a first run at codifying our existing practice here: https://reviews.llvm.org/D80947, which was then committed after significant time on llvm-commits. > > > > I would like to encourage the list via discussion and further reviews/commits to come to a consensus on what we actually MEAN by this rule. For example, a recent comment points out that : > > > > If (cond) > > Stmt; > > else if (cond) > > Stmt; > > else { > > Stmt; > > Stmt; > > } > > > > Should require braces on all of the conditions! However, we are extraordinarily inconsistent here. My wish is for us to become more consistent, so I would like us to use this thread to organize our collective thoughts on figuring out what the rule actually SHOULD be, and organizing a handful of commits to the coding standard to make sure it says what we mean. >>From my experiance the above is the convention in clang. I would like to add that-Wmisleading-indentation exists and that it is enabled by -Wall. Therefore in practice the bogus: if (cond) foo(); bar(); will be detected. Bruno Ricci> > Thanks, > Erich > > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >
On Mon, Jun 15, 2020 at 3:46 PM Keane, Erich via llvm-dev <llvm-dev at lists.llvm.org> wrote:> > Hi all- > > > > A few weeks ago I noticed that our “omit braces with single line blocks” rule wasn’t written down! Additionally, as a group on IRC and in review, noticed that the enforcement of this rule has been extremely inconsistent. We made a first run at codifying our existing practice here: https://reviews.llvm.org/D80947, which was then committed after significant time on llvm-commits. > > > > I would like to encourage the list via discussion and further reviews/commits to come to a consensus on what we actually MEAN by this rule. For example, a recent comment points out that : > > > > If (cond) > > Stmt; > > else if (cond) > > Stmt; > > else { > > Stmt; > > Stmt; > > } > > > > Should require braces on all of the conditions! However, we are extraordinarily inconsistent here. My wish is for us to become more consistent, so I would like us to use this thread to organize our collective thoughts on figuring out what the rule actually SHOULD be, and organizing a handful of commits to the coding standard to make sure it says what we mean.I'm in the always-use-braces camp, so my preference is that if one of the branches in the chain requires braces, we should use them for all branches. I consider comments to count towards whether braces are required, so: if (foo) // This is bad bar(); if (foo) { // This is good bar(); } if (foo) { bar(); // But I prefer this } ~Aaron
On 6/16/20, 2:14 AM, "llvm-dev on behalf of Bruno Ricci via llvm-dev" <llvm-dev-bounces at lists.llvm.org on behalf of llvm-dev at lists.llvm.org> wrote: ...> I would like to add that > -Wmisleading-indentation exists and that it is enabled by -Wall. Therefore in practice > the bogus: > > if (cond) > foo(); > bar(); > > will be detected.True, unless you happen to run clang-format. Then all evidence of your original intention disappears. Tim