[llvm-dev] Codifying our Brace rules-

[Robinson, Paul via llvm-dev]

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

> 
> Jay.
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev