This is an archive of the discontinued LLVM Phabricator instance.

[Docs] Clarify the guideline on omitting braces
ClosedPublic

Authored by owenpan on May 26 2022, 6:20 PM.

Details

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.

Diff Detail

Event Timeline

owenpan created this revision.May 26 2022, 6:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2022, 6:20 PM
owenpan requested review of this revision.May 26 2022, 6:20 PM
lattner accepted this revision.May 26 2022, 10:12 PM

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.

This revision is now accepted and ready to land.May 26 2022, 10:12 PM

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
1596–1597

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 added inline comments.May 27 2022, 12:25 AM
llvm/docs/CodingStandards.rst
1596–1597

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.

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
1596

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 added inline comments.May 31 2022, 6:35 AM
llvm/docs/CodingStandards.rst
1596–1597

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 added inline comments.
llvm/docs/CodingStandards.rst
1596–1597

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 added inline comments.May 31 2022, 6:56 AM
llvm/docs/CodingStandards.rst
1596–1597

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.

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 added inline comments.May 31 2022, 9:30 AM
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 added inline comments.May 31 2022, 9:52 AM
llvm/docs/CodingStandards.rst
1596–1597
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.

1596–1597

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. ;)

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
1596–1597

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 added inline comments.May 31 2022, 10:12 AM
llvm/docs/CodingStandards.rst
1596–1597

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 added inline comments.May 31 2022, 10:35 AM
llvm/docs/CodingStandards.rst
1596–1597

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 added inline comments.May 31 2022, 10:38 AM
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 added inline comments.May 31 2022, 10:52 AM
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.

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 added inline comments.May 31 2022, 12:24 PM
llvm/docs/CodingStandards.rst
1596–1597

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 added inline comments.May 31 2022, 2:10 PM
llvm/docs/CodingStandards.rst
1596–1597

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 added inline comments.May 31 2022, 2:17 PM
llvm/docs/CodingStandards.rst
1596–1597

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 added inline comments.May 31 2022, 3:10 PM
llvm/docs/CodingStandards.rst
1596–1597

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 updated this revision to Diff 433205.May 31 2022, 3:22 PM

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

owenpan edited the summary of this revision. (Show Details)May 31 2022, 3:28 PM

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

mehdi_amini added inline comments.May 31 2022, 5:37 PM
llvm/docs/CodingStandards.rst
1596–1597

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

mehdi_amini accepted this revision.May 31 2022, 5:39 PM
owenpan added inline comments.May 31 2022, 11:23 PM
llvm/docs/CodingStandards.rst
1596–1597

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.

owenpan marked an inline comment as done.May 31 2022, 11:25 PM
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Jun 1 2022, 1:11 AM
llvm/docs/CodingStandards.rst
1596–1597

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 added inline comments.Jun 1 2022, 1:19 AM
llvm/docs/CodingStandards.rst
1596–1597

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 added inline comments.Jun 1 2022, 2:06 AM
llvm/docs/CodingStandards.rst
1596–1597

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.