thr3ads.net - llvm dev - [llvm-dev] Codifying our Brace rules- [Jun 2020]

If this information is useful, please help other people find it:
Share via:

Adrian McCarthy via llvm-dev

2020-Jun-23 15:51 UTC

[llvm-dev] Codifying our Brace rules-

Personally, I favor "always use braces" because it helps readability
for
me.  The compiler may be good at flagging misleading indentation, but my
visual processing system is terrible at it, especially since we use a
measly two spaces for indentation.  And we grant indentation exceptions
for--among other things--case labels in switches.

When some nested statements have braces and others don't, I can't rely
on
visual pattern matching to grok the control flow at a glance (which is a
primary reason to standardize on brace and indentation styles in the first
place).  Instead I have to work out what goes with what and stuff those
answers into short-term memory as I work with the block.

I doubt we're going to move to wider indentation, and I don't expect
I'll
convince anyone to indent case labels an extra level.  But the "always use
braces" rule is preferred by some here, is self-consistent, and is the only
distinction between LLVM's present (inconsistently applied) brace style and
the One True Brace Style[1].  It's mindless to apply and it helps
compensate for our stingy indentation.  If the vertical space used by close
braces seems too high a cost, I think that's an indication that there may
be too many levels of nesting or the function is too long or both.

[1]:  https://en.wikipedia.org/wiki/Indentation_style#Variant:_1TBS_(OTBS)

On Tue, Jun 23, 2020 at 7:40 AM Robinson, Paul via llvm-dev <
llvm-dev at lists.llvm.org> 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
>
> >
> > Jay.
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200623/461d217b/attachment.html>

David Chisnall via llvm-dev

2020-Jun-24 10:02 UTC

head link

[llvm-dev] Codifying our Brace rules-

On 23/06/2020 16:51, Adrian McCarthy via llvm-dev wrote:> Personally, I favor "always use braces" because it helps
readability for
> me.  The compiler may be good at flagging misleading indentation, but my 
> visual processing system is terrible at it, especially since we use a 
> measly two spaces for indentation.  And we grant indentation exceptions 
> for--among other things--case labels in switches.
I remember fixing one bug in LLVM that was caused by not having braces 
around an if statement and a modification that didn't notice this.  I 
have fixed other bugs in a downstream fork where upstream has done this:

if (x) {
   something();
   somethingElse();
}

We've changed it to:

if (x) {
   something();
   somethingElse();
   ohAndOneMoreThing();
}

And then upstream has changed it to

if (x)
   something();

And, after merge, our code has been:

if (x)
   something();
   ohAndOneMoreThing();

In contrast, I have *never*, in any project, had to fix a bug that was 
caused by someone inserting redundant braces (or extra brackets) and 
someone else failing to understand the code.

As such, I am strongly in favour of requiring extra braces.  Static 
analysers will now often catch the bugs that they prevent but it's 
better to make it harder to introduce bugs in the first place than to 
depend on tooling to find them later.

David

Fangrui Song via llvm-dev

2020-Jun-24 15:17 UTC

head link

[llvm-dev] Codifying our Brace rules-

On 2020-06-24, David Chisnall via llvm-dev wrote:>On 23/06/2020 16:51, Adrian McCarthy via llvm-dev wrote:
>>Personally, I favor "always use braces" because it helps
readability
>>for me.  The compiler may be good at flagging misleading 
>>indentation, but my visual processing system is terrible at it, 
>>especially since we use a measly two spaces for indentation.  And we 
>>grant indentation exceptions for--among other things--case labels in 
>>switches.
>
>I remember fixing one bug in LLVM that was caused by not having braces 
>around an if statement and a modification that didn't notice this.  I 
>have fixed other bugs in a downstream fork where upstream has done 
>this:
>
>if (x) {
>  something();
>  somethingElse();
>}
>
>We've changed it to:
>
>if (x) {
>  something();
>  somethingElse();
>  ohAndOneMoreThing();
>}
>
>And then upstream has changed it to
>
>if (x)
>  something();
>
>And, after merge, our code has been:
>
>if (x)
>  something();
>  ohAndOneMoreThing();
>
>In contrast, I have *never*, in any project, had to fix a bug that was 
>caused by someone inserting redundant braces (or extra brackets) and 
>someone else failing to understand the code.
>
>As such, I am strongly in favour of requiring extra braces.  Static 
>analysers will now often catch the bugs that they prevent but it's 
>better to make it harder to introduce bugs in the first place than to 
>depend on tooling to find them later.
>
>David
First, there is a merge conflict:

--- i/a.c
+++ w/a.c
@@@ -1,2 -1,5 +1,8 @@@
  -if (x) {
  +if (x)
      something();
++<<<<<<< HEAD
++======+    somethingElse();
+    onemorething();
+ }
++>>>>>>> 78750e9... onemore


Second, both GCC>=6 and clang (https://reviews.llvm.org/D70638 ) -Wall enable
-Wmisleading-indentation to catch the issue.

Mehdi AMINI via llvm-dev

2020-Jun-24 16:08 UTC

head link

[llvm-dev] Codifying our Brace rules-

On Wed, Jun 24, 2020 at 3:02 AM David Chisnall via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> On 23/06/2020 16:51, Adrian McCarthy via llvm-dev wrote:
> > Personally, I favor "always use braces" because it helps
readability for
> > me.  The compiler may be good at flagging misleading indentation, but
my
> > visual processing system is terrible at it, especially since we use a
> > measly two spaces for indentation.  And we grant indentation
exceptions
> > for--among other things--case labels in switches.
>
> I remember fixing one bug in LLVM that was caused by not having braces
> around an if statement and a modification that didn't notice this.  I
> have fixed other bugs in a downstream fork where upstream has done this:
>
> if (x) {
>    something();
>    somethingElse();
> }
>
> We've changed it to:
>
> if (x) {
>    something();
>    somethingElse();
>    ohAndOneMoreThing();
> }
>
> And then upstream has changed it to
>
> if (x)
>    something();
>
> And, after merge, our code has been:
>
> if (x)
>    something();
>    ohAndOneMoreThing();
>
To be clear this was a merge-conflict that was manually resolved this way
right? I don't see git auto-merging this diff.

> In contrast, I have *never*, in any project, had to fix a bug that was
> caused by someone inserting redundant braces (or extra brackets) and
> someone else failing to understand the code.
>
> As such, I am strongly in favour of requiring extra braces.  Static
> analysers will now often catch the bugs that they prevent but it's
> better to make it harder to introduce bugs in the first place than to
> depend on tooling to find them later.
>
> David
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200624/1dc80a5a/attachment.html>

llvm dev - Jun 2020 - Codifying our Brace rules-

[llvm-dev] Codifying our Brace rules-

[llvm-dev] Codifying our Brace rules-

[llvm-dev] Codifying our Brace rules-

[llvm-dev] Codifying our Brace rules-