[Docs] Clarify the guideline on omitting braces

[owenpan]

Summary

While working on a clang-format option RemoveBracesLLVM that removes braces following the guideline, we were unsure about what to do with the braces of do-while loops. (See D126157#inline-1209965.) The ratio of using to omitting the braces is about 4:1 in the llvm-project source, so it will help to add an example to the guideline.

Also cleans up the original examples including making the nested if example more targeted on avoiding potential dangling else situations.

[lattner]

This makes sense to me. do/while loops are rare, and because the condition comes after the body, they are structurally different than the other loops and conditionals.

[mehdi_amini]

In D126512#3541698, @lattner wrote:

This makes sense to me. do/while loops are rare, and because the condition comes after the body, they are structurally different than the other loops and conditionals.

Did you notice the other change in the revision?

llvm/docs/CodingStandards.rst

1601–1602

This is new and seems overly restrictive to me, why this new change?

1633

This example shows how this revision goes beyond do/while.

[owenpan]

llvm/docs/CodingStandards.rst

1601–1602

See the second paragraph of the patch summary above. The phrase "complex enough" is too subjective to be useful IMO, but I will withdraw this specific change if you insist.

1633

This example was very similar to the one starting at line 1662. After taking out one level of nesting here, it won't seem redundant to the "Use braces on the outer block because there are more than two levels of nesting" example below.

[erichkeane]

I'd prefer we limit the scope of this to JUST a 'do-while' loop. I think we can clarify the "if an inner statement uses braces, the outer one should too", AND special-case the do-while loop as needing braces.

BUT THIS, for example:

if (isa<VarDecl>(D))
   for (auto *A : D.attrs())
     if (shouldProcessAttr(A))
       handleAttr(A);

Which I think the 'single line' rule you post breaks. Also, my interpretation of what you wrote ALSo breaks 1649 in the 'after'.

should remain the way.

llvm/docs/CodingStandards.rst

1601

I'm not really a fan of this one. I DO really like the 'cascading single-liners', and want to keep those omitting braces. The example @mehdi_amini pointed out is important to me to keep.

[mehdi_amini]

llvm/docs/CodingStandards.rst

1601–1602

Subjective isn't always bad :)
The coding standard is designed to leave some room indeed when we can't specify a rule that is always clear cut.

1633

I rather keep this example though if we don’t have a good reason to remove it

[aaron.ballman]

llvm/docs/CodingStandards.rst

1601–1602

I'd like to make sure we're all reading this the same way, because I don't see this as being restrictive so much as formalizing what I think many reviewers have been asking for anyway. Here's some examples:

if (foo)  // Old rule: didn't ask for braces, new rule: asks for braces
  bar(1, 2,
        3, 4);

if (foo)  // Old rule: didn't ask for braces, new rule: asks for braces
  bar(1, 2,
        3, 4);
else  // Old rule: didn't ask for braces, new rule: doesn't ask for braces
  baz(1, 2);

if (foo) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
  baz(1, 2);
else  // Old rule: didn't ask for braces, new rule: asks for braces
  bar(1, 2,
        3, 4);

if (foo) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
  if (bar) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
    baz(1, 2);

if (foo) // Old rule: didn't ask for braces, new rule: asks for braces
  if (bar(1, 2,
            3, 4)) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
    baz(1, 2);

if (foo) // Old rule: didn't ask for braces, new rule: asks for braces
  for (int i = 0; i < 10; ++i) // Old rule: didn't ask for braces, new rule: asks for braces
    baz(i);

Am I interpreting this correctly?

(FWIW, I personally think it's better for us to err on recommending braces when they're unnecessary in these more complicated situations because that's the safer, more explicit style.)

[mehdi_amini]

llvm/docs/CodingStandards.rst

1601–1602

Your examples are on point and are the reasons why I see this as a non-trivial policy change. At minima this requires a thread on discourse and a revision separate from the do/while case.

[owenpan]

In D126512#3547201, @erichkeane wrote:

BUT THIS, for example:

if (isa<VarDecl>(D))
   for (auto *A : D.attrs())
     if (shouldProcessAttr(A))
       handleAttr(A);

Which I think the 'single line' rule you post breaks. Also, my interpretation of what you wrote ALSo breaks 1649 in the 'after'.

should remain the way.

It should not affect the examples you mentioned, but now I can see how it might be interpreted otherwise. I think removing the sentence "An `if/else` chain or a loop is considered
a single statement for this rule, and this rule applies recursively" would help. (See D126157 for some examples it intends to clarify.)

llvm/docs/CodingStandards.rst

1633

You would use the braces on the outer if in the original example regardless whether there was a potential dangling else situation because it had two levels of nesting. The revision removes the duplication. (In clang-format unit tests, this example doesn't really test the rule because of the multilevel nesting one below.)

[erichkeane]

llvm/docs/CodingStandards.rst

1633

Where in the prose do you see the '2 levels of nesting rule'? I don't see anything like that in the rules...

[owenpan]

llvm/docs/CodingStandards.rst

1601–1602

if (foo)  // Old rule: didn't ask for braces, new rule: asks for braces
  bar(1, 2,
        3, 4);
else  // Old rule: didn't ask for braces, new rule: doesn't ask for braces
  baz(1, 2);

The new rule asks for braces on the if (and because of the existing rule on 1636) and on the else.

if (foo) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
  baz(1, 2);
else  // Old rule: didn't ask for braces, new rule: asks for braces
  bar(1, 2,
        3, 4);

Similarly, the new rule asks for braces on both if and else.

if (foo) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
  if (bar) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
    baz(1, 2);

Both old and new rules ask for braces on the outer if, but the old example didn't really convey that.

if (foo) // Old rule: didn't ask for braces, new rule: asks for braces
  if (bar(1, 2,
            3, 4)) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
    baz(1, 2);

Both old and new rules ask braces on the outer if.

if (foo) // Old rule: didn't ask for braces, new rule: asks for braces
  for (int i = 0; i < 10; ++i) // Old rule: didn't ask for braces, new rule: asks for braces
    baz(i);

Both old and new rules don't ask for no braces.

1601–1602

I agree that some subjectivity in some situations are a good thing, but "complex enough" is too vague to be useful here. I think this rule can be clear cut as it has already been implemented in clang-format. ;)

[owenpan]

I will undo the clarification on complex single-statement body but keep the change to the potential dangling else example if everyone is ok with that.

llvm/docs/CodingStandards.rst

1601–1602

if (foo) // Old rule: didn't ask for braces, new rule: asks for braces

for (int i = 0; i < 10; ++i) // Old rule: didn't ask for braces, new rule: asks for braces
  baz(i);

Both old and new rules don't ask for no braces.

I meant "neither the old nor the new rule ask for braces."

1633

Where in the prose do you see the '2 levels of nesting rule'? I don't see anything like that in the rules...

Line 1662:

// Use braces on the outer block because there are more than two levels of nesting.

[aaron.ballman]

llvm/docs/CodingStandards.rst

1601–1602

if (foo) // Old rule: didn't ask for braces, new rule: asks for braces
bar(1, 2,

3, 4);

else // Old rule: didn't ask for braces, new rule: doesn't ask for braces

baz(1, 2);

The new rule asks for braces on the if (and because of the existing rule on 1636) and on the else.

Ah, yeah, I don't think that existing rule is sound. We have a *ton* of examples where code does:

if (foo)
  ...
else if (bar)
  ...
else if (baz)
  ...
...
else {
  // Error handling
}

and requiring braces on the entire chain just because something, somewhere in the chain has braces doesn't really change the readability. However, let's not debate the words already in the coding guidelines just yet and take it on faith we actually intended that rule.

if (foo) // Old rule: didn't ask for braces, new rule: doesn't ask for braces

baz(1, 2);

else // Old rule: didn't ask for braces, new rule: asks for braces

bar(1, 2,
      3, 4);

Similarly, the new rule asks for braces on both if and else.

Agreed.

if (foo) // Old rule: didn't ask for braces, new rule: doesn't ask for braces

if (bar) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
  baz(1, 2);

Both old and new rules ask for braces on the outer if, but the old example didn't really convey that.

Personally, I'm okay with that interpretation, but the old rules were conflicting. We have one example with if statements that suggests only braces on the outer if, and we have another example with an if followed by for that suggests no braces on anything.

if (foo) // Old rule: didn't ask for braces, new rule: asks for braces

if (bar(1, 2,
          3, 4)) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
  baz(1, 2);

Both old and new rules ask braces on the outer if.

I'm okay with this interpretation (but it's debatable on the old rules from above).

if (foo) // Old rule: didn't ask for braces, new rule: asks for braces

for (int i = 0; i < 10; ++i) // Old rule: didn't ask for braces, new rule: asks for braces
  baz(i);

Both old and new rules don't ask for no braces.

I think the old rules explicitly didn't ask for braces (there's an example on line 1649), but I'm fine if the new rule is now suggesting braces.

However, I don't think the answers are clear based on the prose, especially given how many I got wrong. :-)

[owenpan]

llvm/docs/CodingStandards.rst

1601–1602

if (foo) // Old rule: didn't ask for braces, new rule: doesn't ask for braces

if (bar) // Old rule: didn't ask for braces, new rule: doesn't ask for braces
  baz(1, 2);

Both old and new rules ask for braces on the outer if, but the old example didn't really convey that.

Personally, I'm okay with that interpretation, but the old rules were conflicting. We have one example with if statements that suggests only braces on the outer if, and we have another example with an if followed by for that suggests no braces on anything.

I think the rules and examples are somewhat convoluted, but these two examples don't conflict. The former has a nested if, so a potential dangling else situation might exist when an else is to be added later (whereas the compiler will warn only after an else has been added). The latter example has no nested if, so the braces should be omitted there.

if (foo) // Old rule: didn't ask for braces, new rule: asks for braces

for (int i = 0; i < 10; ++i) // Old rule: didn't ask for braces, new rule: asks for braces
  baz(i);

Both old and new rules don't ask for no braces.

I think the old rules explicitly didn't ask for braces (there's an example on line 1649), but I'm fine if the new rule is now suggesting braces.

Both the old and new rules ask for braces to be omitted because there is no if nested in another if (directly or indirectly) and there are less than two levels of nesting.

[erichkeane]

llvm/docs/CodingStandards.rst

1633

Strange, that one is added JUST in example?! I don't think we've been enforcing that anywhere. I wonder if we treat examples as normative in this document...

[owenpan]

llvm/docs/CodingStandards.rst

1633

Strange, that one is added JUST in example?! I don't think we've been enforcing that anywhere. I wonder if we treat examples as normative in this document...

Yeah, and there are a few more that are only in the examples but not in the prose.

[erichkeane]

I agree with the above, we probably need an RFC on discourse for this.

llvm/docs/CodingStandards.rst

1633

Hrmph... that really frustrates me. I don't think that should be within the power of examples unless they can be backed up. Perhaps we need a FURTHER clarification to the rules for that.

That said, these are guidance, and I don't really care too much, as long as we are consistent.

[mehdi_amini]

llvm/docs/CodingStandards.rst

1601–1602

I agree that some subjectivity in some situations are a good thing, but "complex enough" is too vague to be useful here. I think this rule can be clear cut as it has already been implemented in clang-format. ;)

If clang-format is adding braces in places where the guidelines don't call for it, this is a bug in clang-format and not in the guidelines... (I'm not sure I understood your argument here)

1633

I think this was covered under this text:

This list is not exhaustive, for example, readability is also harmed if an

`if/else` chain does not use braced bodies for either all or none of its
members, with complex conditionals, deep nesting, etc. The examples below intend to provide some guidelines.

Unfortunately "deep nesting" is subjective, and the two-levels was added as a guideline in the example instead of making the text clear.
I don't have a specific opinion on the level of nesting, I suspect that I intuitively would use braces when the statement isn't single-line and there is more than one level of nesting.

[owenpan]

llvm/docs/CodingStandards.rst

1601–1602

I agree that some subjectivity in some situations are a good thing, but "complex enough" is too vague to be useful here. I think this rule can be clear cut as it has already been implemented in clang-format. ;)

If clang-format is adding braces in places where the guidelines don't call for it, this is a bug in clang-format and not in the guidelines... (I'm not sure I understood your argument here)

My point is that the prose should be made more concrete on this rule so that developers/reviewers/clang-format can follow. We don't want/need a long debate every time some say the single-statement body is "complex enough" while the others argue the opposite.

clang-format has inserted hundreds of braces in D126157 based on whether an unwrapped line can fit on a single line. Would you have omitted braces on any of them?

1633

I think this was covered under this text:

This list is not exhaustive, for example, readability is also harmed if an

`if/else` chain does not use braced bodies for either all or none of its
members, with complex conditionals, deep nesting, etc. The examples below intend to provide some guidelines.

Unfortunately "deep nesting" is subjective, and the two-levels was added as a guideline in the example instead of making the text clear.
I don't have a specific opinion on the level of nesting, I suspect that I intuitively would use braces when the statement isn't single-line and there is more than one level of nesting.

I see. Perhaps we should change "deep nesting" to "more than two levels of nesting" in a separate revision.

[mehdi_amini]

llvm/docs/CodingStandards.rst

1601–1602

clang-format has inserted hundreds of braces in D126157 based on whether an unwrapped line can fit on a single line. Would you have omitted braces on any of them?

Yes: most of them I think.

[owenpan]

llvm/docs/CodingStandards.rst

1601–1602

clang-format has inserted hundreds of braces in D126157 based on whether an unwrapped line can fit on a single line. Would you have omitted braces on any of them?

Yes: most of them I think.

Can you just pick one of them for which you would omit the braces so that we can have an idea what kind of wrapped lines you would consider not complex enough? And also give an opposite example that you would consider complex enough even though the line would not wrap?

[owenpan]

Dropped the controversial clarification on complex single-statement body. Also cleaned up the original examples (indentation, column limit, grammar, etc.)

[lattner]

I agree this should be limited to do/while loops to make progress.

[mehdi_amini]

llvm/docs/CodingStandards.rst

1601–1602

Sure, here is an example where I don't feel the need for braces quite equally between these:

if (Current.MustBreakBefore || Current.is(TT_InlineASMColon))
  return true;
if (CurrentState.BreakBeforeClosingBrace &&
    Current.closesBlockOrBlockTypeList(Style))
  return true;

I'm fairly sure we have plenty of recent code written this way.
Here is a pretty extreme example: https://github.com/llvm/llvm-project/blob/main/mlir/lib/Dialect/Vector/IR/VectorOps.cpp#L423-L433

It's harder to find example of the opposite, but one may be when a multi-line body with an else seems like more natural to have braces: https://github.com/llvm/llvm-project/blob/main/mlir/lib/Conversion/MathToLLVM/MathToLLVM.cpp#L109-L113

[owenpan]

llvm/docs/CodingStandards.rst

1601–1602

Sure, here is an example where I don't feel the need for braces quite equally between these:

if (Current.MustBreakBefore || Current.is(TT_InlineASMColon))
  return true;
if (CurrentState.BreakBeforeClosingBrace &&
    Current.closesBlockOrBlockTypeList(Style))
  return true;

It seems that the number of binary logical operators has an impact on your preference of using/omitting braces. OTOH I'd rather look at the fact that the body of the second if doesn't immediately follow the if and thus would insert braces.

I'm fairly sure we have plenty of recent code written this way.
Here is a pretty extreme example: https://github.com/llvm/llvm-project/blob/main/mlir/lib/Dialect/Vector/IR/VectorOps.cpp#L423-L433

I'm not sure if the braces were omitted intentionally. There also exist a lot of examples where elidable braces were not removed. Anyway, a concrete and unambiguous rule would help to reduce inconsistencies between using and omitting braces.

It's harder to find example of the opposite, but one may be when a multi-line body with an else seems like more natural to have braces: https://github.com/llvm/llvm-project/blob/main/mlir/lib/Conversion/MathToLLVM/MathToLLVM.cpp#L109-L113

I would interpret it differently: use braces for the else block to "keep it uniform" with the if block.

1633

I think this was covered under this text:

This list is not exhaustive, for example, readability is also harmed if an

`if/else` chain does not use braced bodies for either all or none of its
members, with complex conditionals, deep nesting, etc. The examples below intend to provide some guidelines.

Unfortunately "deep nesting" is subjective, and the two-levels was added as a guideline in the example instead of making the text clear.
I don't have a specific opinion on the level of nesting, I suspect that I intuitively would use braces when the statement isn't single-line and there is more than one level of nesting.

I see. Perhaps we should change "deep nesting" to "more than two levels of nesting" in a separate revision.

[mehdi_amini]

llvm/docs/CodingStandards.rst

1601–1602

It seems that the number of binary logical operators has an impact on your preference of using/omitting braces.

Not really: I was just giving examples of multi-line if without braces.

I would interpret it differently: use braces for the else block to "keep it uniform" with the if block.

The if block didn't need braces here...

[owenpan]

llvm/docs/CodingStandards.rst

1601–1602

I would interpret it differently: use braces for the else block to "keep it uniform" with the if block.

The if block didn't need braces here...

I would definitely use braces for the if though because the single simple statement body was split into not just two, but three lines. :)

[mehdi_amini]

llvm/docs/CodingStandards.rst

1601–1602

I'm sure you would, but I was answering your question about a complex case: I suspect that it is actually the multi-line in combination with an else that lead to have braces here to improve readability.

llvm/docs/CodingStandards.rst

 When you are looking at "``runHelper``" in the middle of a large C++ file,
 you have no immediate way to tell if this function is local to the file.  In
 contrast, when the function is marked static, you don't need to cross-reference
 faraway places in the file to tell that the function is local.
 
 Don't Use Braces on Simple Single-Statement Bodies of if/else/loop Statements
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-When writing the body of an ``if``, ``else``, or loop statement, we prefer to
-omit the braces to avoid unnecessary line noise. However, braces should be used
-in cases where the omission of braces harm the readability and maintainability
-of the code.
+When writing the body of an ``if``, ``else``, or for/while loop statement, we
+prefer to omit the braces to avoid unnecessary line noise. However, braces
+should be used in cases where the omission of braces harm the readability and
+maintainability of the code.
 
 We consider that readability is harmed when omitting the brace in the presence
 of a single statement that is accompanied by a comment (assuming the comment
 can't be hoisted above the ``if`` or loop statement, see below).
+
 Similarly, braces should be used when a single-statement body is complex enough
 that it becomes difficult to see where the block containing the following
 statement began. An ``if``/``else`` chain or a loop is considered a single
 statement for this rule, and this rule applies recursively.
 
-This list is not exhaustive, for example, readability is also harmed if an
+This list is not exhaustive. For example, readability is also harmed if an
 ``if``/``else`` chain does not use braced bodies for either all or none of its
-members, with complex conditionals, deep nesting, etc. The examples below
+members, or has complex conditionals, deep nesting, etc. The examples below
 intend to provide some guidelines.
 
 Maintainability is harmed if the body of an ``if`` ends with a (directly or
 indirectly) nested ``if`` statement with no ``else``. Braces on the outer ``if``
 would help to avoid running into a "dangling else" situation.
 
 
 .. code-block:: c++
 
-  // Omit the braces, since the body is simple and clearly associated with the if.
+  // Omit the braces since the body is simple and clearly associated with the
+  // `if`.
   if (isa<FunctionDecl>(D))
     handleFunctionDecl(D);
   else if (isa<VarDecl>(D))
     handleVarDecl(D);
 
-
   // Here we document the condition itself and not the body.
   if (isa<VarDecl>(D)) {
     // It is necessary that we explain the situation with this surprisingly long
     // comment, so it would be unclear without the braces whether the following
     // statement is in the scope of the `if`.
     // Because the condition is documented, we can't really hoist this
-    // comment that applies to the body above the if.
+    // comment that applies to the body above the `if`.
     handleOtherDecl(D);
   }
 
-  // Use braces on the outer `if` to avoid a potential dangling else situation.
+  // Use braces on the outer `if` to avoid a potential dangling `else`
+  // situation.
   if (isa<VarDecl>(D)) {
-    for (auto *A : D.attrs())
-      if (shouldProcessAttr(A))
-        handleAttr(A);
   }
 
-  // Use braces for the `if` block to keep it uniform with the else block.
+  // Use braces for the `if` block to keep it uniform with the `else` block.
   if (isa<FunctionDecl>(D)) {
     handleFunctionDecl(D);
   } else {
-    // In this else case, it is necessary that we explain the situation with this
-    // surprisingly long comment, so it would be unclear without the braces whether
-    // the following statement is in the scope of the `if`.
+    // In this `else` case, it is necessary that we explain the situation with
+    // this surprisingly long comment, so it would be unclear without the braces
+    // whether the following statement is in the scope of the `if`.
     handleOtherDecl(D);
   }
 
-  // This should also omit braces.  The `for` loop contains only a single statement,
-  // so it shouldn't have braces.  The `if` also only contains a single simple
-  // statement (the for loop), so it also should omit braces.
+  // This should also omit braces.  The `for` loop contains only a single
+  // statement, so it shouldn't have braces.  The `if` also only contains a
+  // single simple statement (the `for` loop), so it also should omit braces.
   if (isa<FunctionDecl>(D))
     for (auto *A : D.attrs())
       handleAttr(A);
 
+  // Use braces for a `do-while` loop and its enclosing statement.
+  if (Tok->is(tok::l_brace)) {
+    do {
+      Tok = Tok->Next;
+    } while (Tok);
+  }
+
   // Use braces for the outer `if` since the nested `for` is braced.
   if (isa<FunctionDecl>(D)) {
     for (auto *A : D.attrs()) {
-      // In this for loop body, it is necessary that we explain the situation
+      // In this `for` loop body, it is necessary that we explain the situation
       // with this surprisingly long comment, forcing braces on the `for` block.
       handleAttr(A);
     }
   }
 
-  // Use braces on the outer block because there are more than two levels of nesting.
+  // Use braces on the outer block because there are more than two levels of
+  // nesting.
   if (isa<FunctionDecl>(D)) {
     for (auto *A : D.attrs())
       for (ssize_t i : llvm::seq<ssize_t>(count))
-         handleAttrOnDecl(D, A, i);
   }
 
-  // Use braces on the outer block because of a nested `if`, otherwise the
+  // Use braces on the outer block because of a nested `if`; otherwise the
   // compiler would warn: `add explicit braces to avoid dangling else`
   if (auto *D = dyn_cast<FunctionDecl>(D)) {
     if (shouldProcess(D))
       handleVarDecl(D);
     else
       markAsIgnored(D);
   }