Codifying our Brace rules-

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

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.

Matt Arsenault via llvm-dev <llvm-dev@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

I personally prefer always including the braces, but, of course, I’ll abide by the local style rules. That said, is the local style rule implemented in clang-format? I’ve never noticed it removing nor adding syntactically unnecessary braces.

A few times, I’ve had a bit of a panic attack on code of this form:

if (condition)
// there’s a comment here
Statement = Something;
else
ComputeThisOrThat();

It’s correct, but it looks like it’s not, especially when it’s nested within a clause of another “outer” if statement.

I personally prefer always including the braces, but, of course, I'll abide by the local style rules. That said, is the local style rule implemented in clang-format? I've never noticed it removing nor adding syntactically unnecessary braces.

Clang-format generally doesn't remove/add tokens (reordering headers
is about the most token-changing thing it does, I think) - so I don't
think it has this feature/would have this feature.

A few times, I've had a bit of a panic attack on code of this form:

if (condition)
  // there's a comment here
  Statement = Something;
else
  ComputeThisOrThat();

It's correct, but it looks like it's not, especially when it's nested within a clause of another "outer" if statement.

Yeah, that's I think the other point of contention/variation in the
LLVM project - single statement, multi-line (either comments, or a
single statement that line wraps) blocks. Some folks put braces, some
don't.

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.

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?

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?

Generally, any time there is a comment within the body I don’t really see it as “trivial” anymore.

Prefer:
if (…) {
// Some comment.
single statement;
}
// Some comment.
If (…)
single statement;

Over:
if (…)
// Some comment
single statement;

– River

+1

-Chris

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

I also prefer to always use braces even when not strictly necessary, for the same reasons as already mentioned. The only situation where I personally choose to remove the braces is when the statement fits on the same line as the condition, e.g.:

if (cond) statement;

In such cases, I find it clear that I need to add braces when expanding the statements.

But in case we want to be on the safe side I vote for always including braces, no matter what.

Gabriel

My 2 pennies is braces add unnecessary clutter and impair readability when
used on a single-line statement. I count comments, that are on their
own line as statement(s).

For example:

BAD:

if (cond)
// Comment
foo();

GOOD:

if (cond) {
// Comment
foo();
}

BAD:
if (cond) {
foo(); // Comment
}

GOOD:

if (cond)
foo(); // Comment

BAD:
if (cond)
for(;:wink:
foo()

GOOD:
if (cond) {

for(;:wink:
foo()

}

Some new-ish languages like Go and Swift went to always require braces. However, I’ve never
seen a study, which concluded that always requiring braces has an overall positive effect
on code quality.

Is there such a thing?

Lacking that, it becomes a matter of personal taste and preference and anecdotal evidence
in favour of one or the other style. Speaking of which, as I constantly run clang-format
on the blocks of code, that I currently modify, I don’t think I’ve ever misplaced a statement.

~chill

Hi,

FWIW, I'm on the always-add-braces camp, for the already mentioned
reasons and also the following:
* It makes it easier to add and remove one-off debug code that you
don't intend to commit
* It makes it easier for my editor to jump over the whole block while
scrolling back and forth around code

I also like chill's suggestion, since it at least solves the second issue.

Cheers,
Diana

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

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

...

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

...

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.

Ah yes, that's a good point indeed.

Bruno

I used to agree with the “always put braces unless it fits on a single line” camp, but it turns out that it’s really hard to break in a debugger on statement; if you write it if (cond) statement;

Personally, I’m in the “always put braces” camp. I think the many advantages to this approach have been stated by many people in this thread. The sole (that I’ve seen mentioned) disadvantage is “clutter”. However, I think in practice you learn to ignore the excess braces, especially since we write them like this:


if (foo) {

bar;

} else if (baz) {

quux;

}

That’s only one more line than:


if (foo)

bar;

else if (baz)

quux;

I had never mentioned anything because I have traumatic memories of endless arguments about coding style at previous orgs, but I would personally be strongly be in favor of the rule being “always put braces for the bodies of branches and loops”. Regardless, I think for the purposes of a body being “trivial”, comments on their own line, or statements broken across multiple lines should not be considered trivial:


if (foo)

return nullptr; // this body is trivial

if (foo)

// this body is no longer trivial

return nullptr;

if (bar)

GlobalState =

doTheThing(this, body, is, no, longer, trivial);

Thanks,

Christopher Tetreault

+1

Philip