[llvm-dev] Codifying our Brace rules-

[Alexander Richardson via llvm-dev]

On Wed, 24 Jun 2020 at 00:49, Fangrui Song via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>
> On 2020-06-23, Hal Finkel via llvm-dev wrote:
> >
> >On 6/23/20 9:39 AM, Robinson, Paul via llvm-dev wrote:
> >>
> >>>-----Original Message-----
> >>>From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of Jay Foad via
> >>>llvm-dev
> >>>Sent: Tuesday, June 23, 2020 4:47 AM
> >>>To: Mehdi AMINI <joker.eph at gmail.com>
> >>>Cc: llvm-dev at lists.llvm.org; Matt Arsenault <arsenm2 at gmail.com>
> >>>Subject: Re: [llvm-dev] Codifying our Brace rules-
> >>>
> >>>On Tue, 23 Jun 2020 at 03:30, Mehdi AMINI via llvm-dev
> >>><llvm-dev at lists.llvm.org> wrote:
> >>>>On Mon, Jun 22, 2020 at 2:38 PM Steve Scalpone via llvm-dev <llvm-
> >>>dev at lists.llvm.org> wrote:
> >>>>>Me?  I would modify the first sentence from:
> >>>>>
> >>>>>>When writing the body of an if, else, or loop statement,
> >>>>>>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.
> >>>>>To be:
> >>>>>
> >>>>>>Braces are optional around the body of an if, else, or loop
> >>>statement,
> >>>>>>except in cases where the omission of braces harm the readability and
> >>>>>>maintainability of the code.
> >>>>
> >>>>The current wording is more clear as it expresses unambiguously the
> >>>preferred way of formatting the code. I don't see a benefit to this change
> >>>of phrasing (on the opposite, I prefer less ambiguous).
> >>>
> >>>I really don't like the current wording. It reads like "Do X. However,
> >>>don't do X if ..." (where X is omit braces). I do not find this
> >>>unambiguous! And it makes me unsure how to interpret "to avoid
> >>>unnecessary line noise: did it really mean "omit the braces /if/ that
> >>>avoids unnecessary line noise"?
> >>Given the standard-mandated "line noise" required for lambdas, I
> >>don't think avoiding line noise is much of an argument here.  If
> >>you dislike line noise, C++ in general will annoy you.  As LLVM is
> >>coded in C++, we have to live with it.
> >>
> >>If we're not going to mandate braces-always, we need guidance on
> >>how to resolve a difference of aesthetic opinion between an author
> >>and reviewer.  I've reviewed code where the author used a lot of
> >>technically unnecessary parentheses in a long expression, where I
> >>thought it reduced clarity and he thought it improved clarity; in
> >>cases like this, the most insistent person wins, with a slight edge
> >>to the reviewer who can refuse to LGTM the patch.  "Most insistent
> >>person wins" isn't really a great basis for deciding on coding styles.
> >>
> >>A mix of aesthetic opinions in successive reviewers is what's most
> >>likely to lead to a random mix of styles within a function/module,
> >>and THAT will reduce clarity more than anything.
> >>
> >>The way around this is to suck up our aesthetic preferences and go
> >>with hard-and-fast rules.  A few likely rule sets here:
> >>
> >>(1) always use braces, period.  This would be the easy to remember
> >>and enforce, with some plusses as mentioned previously for IDEs and
> >>not confusing clang-format into "fixing incorrect indentation" when
> >>the problem is actually missing braces.
> >>
> >>(2) always use braces except for a one-line block.  This means
> >>       for (blah)      // BAD
> >>         if (yadayada)
> >>           do_a_thing;
> >>       for (blah) {    // GOOD
> >>         if (yadayada)
> >>           do_a_thing;
> >>       }
> >>
> >>(3) always use braces except for a one-line statement, applied
> >>recursively.  This means
> >>       for (blah)      // GOOD
> >>         for (whatever)
> >>           if (yadayada)
> >>             do_a_thing;
> >>
> >>Note that (2) and (3) would also need to lay down the law with
> >>respect to if-else cases (each branch considered independently,
> >>or all branches must be handled the same way).
> >>
> >>I acknowledge the irony that choosing between the above rules is
> >>primarily an aesthetic choice when the goal was to eliminate
> >>aesthetic choices; however, this is an aesthetic choice made by
> >>the coding standard and *not* by the reviewer, which is my goal.
> >>
> >>--paulr
>
> I am in the (3) camp. As Hubert mentioned, this can omit multiple levels of }:
>
> ```
>        }
>      }
>    }
> ```
>
> (1) "always use braces" is very annoying and greatly impairs my comprehension
> of a block of code.
>
> In Adrian's example, if there is a comment, I'd prefer braces:
>
>    if (condition) {
>      // there's a comment here
>      Statement = Something;
>    } else {
>      ComputeThisOrThat();
>    }
>
> For Aaron's example, I'd prefer:
>
>    if (foo)
>      bar(); // comment
>
> but I'm not so strongly opposed to
>
>    if (foo) {
>      bar(); // comment
>    }
>

As someone who regularly merges from upstream LLVM into a fork with a
large amount of local changes [1], I have a slight preference for more
braces since it does simplify merging in some cases. It can also
reduce the likelihood of conflicts if we had to add a line to an
if/else that didn't have braces.

I think lack of braces doesn't matter much for early-exit "if
(something) return false;". The merge problems usually happen with
nested loops where braces are used for some nesting levels but not
others.
Another thing that has caused some merge issues in the past are
if/else-if/else chains where braces are not used for all branches:

// BAD:
 if (condition) {
   Statement1;
   Statement2;
} else if (condition2)
   Statement4;
 else
   Statement5;

// GOOD
 if (condition) {
   Statement1;
   Statement2;
 } else {
   Statement2;
}

In terms of reducing merge errors I believe that "(3) always use
braces except for a one-line statement" combined with "if one if/else
branch uses braces, all of them should use braces" would be useful.
Always enforcing braces would obviously also help, but I have a slight
preference for being allowed to omit them for single line statements.


Alex

[1] https://github.com/CTSRD-CHERI/llvm-project - If I compare to the
latest merge, git reports 2391 files changed, 177636 insertions(+),
7818 deletions(-). A lot of this is added tests, but I have never been
able to merge more than about three days worth of upstream changes
without a conflict.