[PATCH] D126512: [Docs] Clarify the guideline on omitting braces

[Mehdi AMINI via Phabricator via llvm-commits]

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



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126512/new/

https://reviews.llvm.org/D126512