[PATCH] D126512: [Docs] Clarify the guideline on omitting braces
Mehdi AMINI via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue May 31 17:37:53 PDT 2022
mehdi_amini added inline comments.
================
Comment at: llvm/docs/CodingStandards.rst:1596-1597
-This list is not exhaustive, for example, readability is also harmed if an
+Similarly, braces should be used when a single-statement body can't fit on a
+single line; otherwise, it would be difficult to see where the block containing
+the following statement began. An ``if``/``else`` chain or a loop is considered
----------------
owenpan wrote:
> mehdi_amini wrote:
> > owenpan wrote:
> > > mehdi_amini wrote:
> > > > owenpan wrote:
> > > > > aaron.ballman wrote:
> > > > > > owenpan wrote:
> > > > > > > owenpan wrote:
> > > > > > > > owenpan wrote:
> > > > > > > > > mehdi_amini wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > erichkeane wrote:
> > > > > > > > > > > > mehdi_amini wrote:
> > > > > > > > > > > > > owenpan wrote:
> > > > > > > > > > > > > > mehdi_amini wrote:
> > > > > > > > > > > > > > > This is new and seems overly restrictive to me, why this new change?
> > > > > > > > > > > > > > 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.
> > > > > > > > > > > > > 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.
> > > > > > > > > > > > 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.
> > > > > > > > > > > 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.)
> > > > > > > > > > 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.
> > > > > > > > > >
> > > > > > > > > > ```
> > > > > > > > > > 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.
> > > > > > > > >
> > > > > > > > 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 (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."
> > > > > > >
> > > > > > >> 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. :-)
> > > > > > >> 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.
> > > > > 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)
> > > > > 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?
> > > 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.
> > > 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?
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
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126512/new/
https://reviews.llvm.org/D126512
More information about the llvm-commits
mailing list