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:

James Henderson via llvm-dev

2020-Jun-16 08:02 UTC

[llvm-dev] Codifying our Brace rules-

I'm with Matt on this one. I much prefer the approach of ALWAYS use braces
for ifs and for loops, even if they're not needed, for basically the same
reasons as he put. The number of times I've added a statement inside an if
without braces and forget to add them is annoyingly high, especially as
it's not always an obvious error upfront. Similarly, being involved in a
downstream codebase with several private patches, having to sometimes add
the braces makes merges all the harder and sometimes more dangerous.

I doubt we're going to get the policy changed from "don't include
unnecessary braces for trivial statements" but if there's any style
that
adds them in more places, I'm up for that.

James

On Mon, 15 Jun 2020 at 20:56, Matt Arsenault via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
>
>
> On Jun 15, 2020, at 15:46, Keane, Erich via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
> Hi all-
>
> A few weeks ago I noticed that our “omit braces with single line blocks”
> rule wasn’t written down!  Additionally, as a group on IRC and in review,
> noticed that the enforcement of this rule has been extremely inconsistent.
> We made a first run at codifying our existing practice here:
> https://reviews.llvm.org/D80947, which was then committed after
> significant time on llvm-commits.
>
> I would like to encourage the list via discussion and further
> reviews/commits to come to a consensus on what we actually MEAN by this
> rule.  For example, a recent comment points out that :
>
> If (cond)
>   Stmt;
> else if (cond)
>   Stmt;
> else {
>   Stmt;
>   Stmt;
> }
>
> Should require braces on all of the conditions!  However, we are
> extraordinarily inconsistent here.  My wish is for us to become more
> consistent, so I would like us to use this thread to organize our
> collective thoughts on figuring out what the rule actually SHOULD be, and
> organizing a handful of commits to the coding standard to make sure it says
> what we mean.
>
> Thanks,
> Erich
>
>
>
> I think braces should be added in all contexts, and the more contexts the
> better. It eliminates any inconsistency or attempt to contextually
> interpret rules. It also reduces merge conflicts, since something
> eventually something will probably be added inside any control flow
> statement. I’ve suffered through many painful merges trying to find where
> the braces went wrong, usually due to switch statements. The
> sometimes-braces-sometimes-not combined with the lack of indentation for
> switch cases leads to way more time figuring out braces than should be
> necessary.
>
> -Matt
> _______________________________________________
> 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/20200616/463a54ad/attachment.html>

Gabriel Hjort Åkerlund via llvm-dev

2020-Jun-16 08:15 UTC

head link

[llvm-dev] Codifying our Brace rules-

I also prefer to always use braces even when not strictly necessary, for the
same reasons as already mentioned. The only situation where I personally choose
to remove the braces is when the statement fits on the same line as the
condition, e.g.:

 

if (cond) statement;

 

In such cases, I find it clear that I need to add braces when expanding the
statements.

 

But in case we want to be on the safe side I vote for always including braces,
no matter what.

 

Gabriel 

 

 

From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of James
Henderson via llvm-dev
Sent: den 16 juni 2020 10:02
To: Matt Arsenault <arsenm2 at gmail.com>
Cc: llvm-dev <llvm-dev at lists.llvm.org>
Subject: Re: [llvm-dev] Codifying our Brace rules-

 

I'm with Matt on this one. I much prefer the approach of ALWAYS use braces
for ifs and for loops, even if they're not needed, for basically the same
reasons as he put. The number of times I've added a statement inside an if
without braces and forget to add them is annoyingly high, especially as it's
not always an obvious error upfront. Similarly, being involved in a downstream
codebase with several private patches, having to sometimes add the braces makes
merges all the harder and sometimes more dangerous.

 

I doubt we're going to get the policy changed from "don't include
unnecessary braces for trivial statements" but if there's any style
that adds them in more places, I'm up for that.

 

James

 

On Mon, 15 Jun 2020 at 20:56, Matt Arsenault via llvm-dev <llvm-dev at
lists.llvm.org <mailto:llvm-dev at lists.llvm.org> > wrote:

 





On Jun 15, 2020, at 15:46, Keane, Erich via llvm-dev <llvm-dev at
lists.llvm.org <mailto:llvm-dev at lists.llvm.org> > wrote:

 

Hi all-

 

A few weeks ago I noticed that our “omit braces with single line blocks” rule
wasn’t written down!  Additionally, as a group on IRC and in review, noticed
that the enforcement of this rule has been extremely inconsistent.  We made a
first run at codifying our existing practice here: 
<https://protect2.fireeye.com/v1/url?k=9efa459c-c04ad804-9efa0507-861fcb972bfc-42265df8dd683c0d&q=1&e=1cccb19d-3ee2-420f-b3a4-89ab398de655&u=https%3A%2F%2Freviews.llvm.org%2FD80947>
https://reviews.llvm.org/D80947, which was then committed after significant time
on llvm-commits.

 

I would like to encourage the list via discussion and further reviews/commits to
come to a consensus on what we actually MEAN by this rule.  For example, a
recent comment points out that :

 

If (cond)

  Stmt;

else if (cond)

  Stmt;

else {

  Stmt;

  Stmt;

}

 

Should require braces on all of the conditions!  However, we are extraordinarily
inconsistent here.  My wish is for us to become more consistent, so I would like
us to use this thread to organize our collective thoughts on figuring out what
the rule actually SHOULD be, and organizing a handful of commits to the coding
standard to make sure it says what we mean.


Thanks,
Erich

 

 

I think braces should be added in all contexts, and the more contexts the
better. It eliminates any inconsistency or attempt to contextually interpret
rules. It also reduces merge conflicts, since something eventually something
will probably be added inside any control flow statement. I’ve suffered through
many painful merges trying to find where the braces went wrong, usually due to
switch statements. The sometimes-braces-sometimes-not combined with the lack of
indentation for switch cases leads to way more time figuring out braces than
should be necessary.

 

-Matt

_______________________________________________
LLVM Developers mailing list
llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org> 
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<https://protect2.fireeye.com/v1/url?k=90b9cffd-ce095265-90b98f66-861fcb972bfc-e3caea4c1054abaf&q=1&e=1cccb19d-3ee2-420f-b3a4-89ab398de655&u=https%3A%2F%2Flists.llvm.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fllvm-dev>

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200616/33a6bf13/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 6320 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200616/33a6bf13/attachment.bin>

Momchil Velikov via llvm-dev

2020-Jun-16 08:35 UTC

head link

[llvm-dev] Codifying our Brace rules-

My 2 pennies is braces add unnecessary clutter and impair readability when
used on a *single-line* statement. I count comments, that are on their
own line as statement(s).

For example:

BAD:

if (cond)
  // Comment
  foo();


GOOD:
if (cond) {
  // Comment
  foo();
}

BAD:
if (cond) {
  foo(); // Comment
}

GOOD:
if (cond)
  foo(); // Comment

BAD:
if (cond)
  for(;;)
    foo()


GOOD:
if (cond) {
  for(;;)
    foo()
}

Some new-ish languages like Go and Swift went to always require braces.
However, I've never
seen a study, which concluded that always requiring braces has an overall
positive effect
on code quality.

Is there such a thing?

Lacking that, it becomes a matter of personal taste and preference and
anecdotal evidence
in favour of one or the other style. Speaking of which, as I constantly run
clang-format
on the blocks of code, that I currently modify, I don't think I've ever
misplaced a statement.

~chill

-- 
Compiler scrub, Arm
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200616/44ad6328/attachment.html>

Chris Tetreault via llvm-dev

2020-Jun-16 17:23 UTC

head link

[llvm-dev] Codifying our Brace rules-

I used to agree with the “always put braces unless it fits on a single line”
camp, but it turns out that it’s really hard to break in a debugger on
`statement;` if you write it `if (cond) statement;`

Personally, I’m in the “always put braces” camp. I think the many advantages to
this approach have been stated by many people in this thread. The sole (that
I’ve seen mentioned) disadvantage is “clutter”. However, I think in practice you
learn to ignore the excess braces, especially since we write them like this:

```
if (foo) {
   bar;
} else if (baz) {
   quux;
}
```

That’s only one more line than:

```
if (foo)
   bar;
else if (baz)
   quux;
```

I had never mentioned anything because I have traumatic memories of endless
arguments about coding style at previous orgs, but I would personally be
strongly be in favor of the rule being “always put braces for the bodies of
branches and loops”. Regardless, I think for the purposes of a body being
“trivial”, comments on their own line, or statements broken across multiple
lines should not be considered trivial:

```
if (foo)
   return nullptr; // this body is trivial

if (foo)
   // this body is no longer trivial
   return nullptr;

if (bar)
   GlobalState       doTheThing(this, body, is, no, longer, trivial);
```

Thanks,
   Christopher Tetreault

From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of Gabriel
Hjort Åkerlund via llvm-dev
Sent: Tuesday, June 16, 2020 1:16 AM
To: jh7370.2008 at my.bristol.ac.uk; Matt Arsenault <arsenm2 at gmail.com>
Cc: llvm-dev at lists.llvm.org
Subject: [EXT] Re: [llvm-dev] Codifying our Brace rules-

I also prefer to always use braces even when not strictly necessary, for the
same reasons as already mentioned. The only situation where I personally choose
to remove the braces is when the statement fits on the same line as the
condition, e.g.:

if (cond) statement;

In such cases, I find it clear that I need to add braces when expanding the
statements.

But in case we want to be on the safe side I vote for always including braces,
no matter what.

Gabriel


From: llvm-dev <llvm-dev-bounces at lists.llvm.org<mailto:llvm-dev-bounces
at lists.llvm.org>> On Behalf Of James Henderson via llvm-dev
Sent: den 16 juni 2020 10:02
To: Matt Arsenault <arsenm2 at gmail.com<mailto:arsenm2 at
gmail.com>>
Cc: llvm-dev <llvm-dev at lists.llvm.org<mailto:llvm-dev at
lists.llvm.org>>
Subject: Re: [llvm-dev] Codifying our Brace rules-

I'm with Matt on this one. I much prefer the approach of ALWAYS use braces
for ifs and for loops, even if they're not needed, for basically the same
reasons as he put. The number of times I've added a statement inside an if
without braces and forget to add them is annoyingly high, especially as it's
not always an obvious error upfront. Similarly, being involved in a downstream
codebase with several private patches, having to sometimes add the braces makes
merges all the harder and sometimes more dangerous.

I doubt we're going to get the policy changed from "don't include
unnecessary braces for trivial statements" but if there's any style
that adds them in more places, I'm up for that.

James

On Mon, 15 Jun 2020 at 20:56, Matt Arsenault via llvm-dev <llvm-dev at
lists.llvm.org<mailto:llvm-dev at lists.llvm.org>> wrote:


On Jun 15, 2020, at 15:46, Keane, Erich via llvm-dev <llvm-dev at
lists.llvm.org<mailto:llvm-dev at lists.llvm.org>> wrote:

Hi all-

A few weeks ago I noticed that our “omit braces with single line blocks” rule
wasn’t written down!  Additionally, as a group on IRC and in review, noticed
that the enforcement of this rule has been extremely inconsistent.  We made a
first run at codifying our existing practice here:
https://reviews.llvm.org/D80947<https://protect2.fireeye.com/v1/url?k=9efa459c-c04ad804-9efa0507-861fcb972bfc-42265df8dd683c0d&q=1&e=1cccb19d-3ee2-420f-b3a4-89ab398de655&u=https%3A%2F%2Freviews.llvm.org%2FD80947>,
which was then committed after significant time on llvm-commits.

I would like to encourage the list via discussion and further reviews/commits to
come to a consensus on what we actually MEAN by this rule.  For example, a
recent comment points out that :

If (cond)
  Stmt;
else if (cond)
  Stmt;
else {
  Stmt;
  Stmt;
}

Should require braces on all of the conditions!  However, we are extraordinarily
inconsistent here.  My wish is for us to become more consistent, so I would like
us to use this thread to organize our collective thoughts on figuring out what
the rule actually SHOULD be, and organizing a handful of commits to the coding
standard to make sure it says what we mean.

Thanks,
Erich


I think braces should be added in all contexts, and the more contexts the
better. It eliminates any inconsistency or attempt to contextually interpret
rules. It also reduces merge conflicts, since something eventually something
will probably be added inside any control flow statement. I’ve suffered through
many painful merges trying to find where the braces went wrong, usually due to
switch statements. The sometimes-braces-sometimes-not combined with the lack of
indentation for switch cases leads to way more time figuring out braces than
should be necessary.

-Matt
_______________________________________________
LLVM Developers mailing list
llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org>
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev<https://protect2.fireeye.com/v1/url?k=90b9cffd-ce095265-90b98f66-861fcb972bfc-e3caea4c1054abaf&q=1&e=1cccb19d-3ee2-420f-b3a4-89ab398de655&u=https%3A%2F%2Flists.llvm.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fllvm-dev>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200616/616596a1/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-