Did this conversation reach a conclusion?
My ad hoc tally says that a slight majority of the responders preferred to fully
brace statements and no one wanted to totally eliminate braces.
The technical arguments for fully braced statements were 1) it's considered
a slightly safer coding style and 2) commit diffs with fully braced statements
may be slightly more to the point.
I didn't register any technical arguments for less-than-fully-braced
statement -- the preference seemed to be aesthetic. I may have missed a
technical argument.
Certainly an "always use braces" rule would be simpler than what's
documented now in the LLVM Coding Standards [1].
Another option would be to make braces a developer's choice, and ask that
those omitting braces please follow the rules documented in [1].
[1]
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
On 6/18/20, 3:56 AM, "llvm-dev on behalf of Nicolai Hähnle via
llvm-dev" <llvm-dev-bounces at lists.llvm.org on behalf of llvm-dev at
lists.llvm.org> wrote:
External email: Use caution opening links or attachments
On Tue, Jun 16, 2020 at 10:35 AM Momchil Velikov via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> 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).
+1 for this. I think braces around single-line statements can be
allowed, but they really shouldn't be mandated, and that's been my
personal policy for reviews. In particular,
if (!is_transform_applicable) {
return {};
}
is very aggravating clutter.
Braces should be required around multi-line statements. Note:
BAD:
for (...)
for (...)
single_line_statement;
GOOD:
for (...) {
for (...)
single_line_statement;
}
Cheers,
Nicolai
--
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.
_______________________________________________
LLVM Developers mailing list
llvm-dev at lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
For those who don’t like it, is the currently documented policy broken enough to be important to changing? I assume you wouldn’t recommend a massive rewrite of the codebase, so we’re going to be with this for quite some time. -Chris> On Jun 22, 2020, at 1:36 PM, Steve Scalpone via llvm-dev <llvm-dev at lists.llvm.org> wrote: > > Did this conversation reach a conclusion? > > My ad hoc tally says that a slight majority of the responders preferred to fully brace statements and no one wanted to totally eliminate braces. > > The technical arguments for fully braced statements were 1) it's considered a slightly safer coding style and 2) commit diffs with fully braced statements may be slightly more to the point. > > I didn't register any technical arguments for less-than-fully-braced statement -- the preference seemed to be aesthetic. I may have missed a technical argument. > > Certainly an "always use braces" rule would be simpler than what's documented now in the LLVM Coding Standards [1]. > > Another option would be to make braces a developer's choice, and ask that those omitting braces please follow the rules documented in [1]. > > [1] https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements > > On 6/18/20, 3:56 AM, "llvm-dev on behalf of Nicolai Hähnle via llvm-dev" <llvm-dev-bounces at lists.llvm.org on behalf of llvm-dev at lists.llvm.org> wrote: > > External email: Use caution opening links or attachments > > > On Tue, Jun 16, 2020 at 10:35 AM Momchil Velikov via llvm-dev > <llvm-dev at lists.llvm.org> wrote: >> 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). > > +1 for this. I think braces around single-line statements can be > allowed, but they really shouldn't be mandated, and that's been my > personal policy for reviews. In particular, > > if (!is_transform_applicable) { > return {}; > } > > is very aggravating clutter. > > Braces should be required around multi-line statements. Note: > > BAD: > for (...) > for (...) > single_line_statement; > > GOOD: > for (...) { > for (...) > single_line_statement; > } > > Cheers, > Nicolai > -- > Lerne, wie die Welt wirklich ist, > aber vergiss niemals, wie sie sein sollte. > _______________________________________________ > 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
On Mon, Jun 22, 2020 at 1:36 PM Steve Scalpone via llvm-dev <llvm-dev at lists.llvm.org> wrote:> > Did this conversation reach a conclusion? > > My ad hoc tally says that a slight majority of the responders preferred to fully brace statements and no one wanted to totally eliminate braces. > > The technical arguments for fully braced statements were 1) it's considered a slightly safer coding style and 2) commit diffs with fully braced statements may be slightly more to the point. > > I didn't register any technical arguments for less-than-fully-braced statement -- the preference seemed to be aesthetic. I may have missed a technical argument.Probably because it's the status quo, not because there aren't such arguments. The most common one is probably "shorter vertical real estate" so you can see more code on one screen/page/etc.> Certainly an "always use braces" rule would be simpler than what's documented now in the LLVM Coding Standards [1].& make a lot of LLVM code non-compliant, so another one of those awkward migration things like naming convention changes.> Another option would be to make braces a developer's choice, and ask that those omitting braces please follow the rules documented in [1].Making code awkwardly inconsistent based on which bits of the code were written by whom - I don't think that'd be ideal. Make for some cognitive dissonance while reading code - you read a few lines written one way, then switch, etc.> [1] https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements > > On 6/18/20, 3:56 AM, "llvm-dev on behalf of Nicolai Hähnle via llvm-dev" <llvm-dev-bounces at lists.llvm.org on behalf of llvm-dev at lists.llvm.org> wrote: > > External email: Use caution opening links or attachments > > > On Tue, Jun 16, 2020 at 10:35 AM Momchil Velikov via llvm-dev > <llvm-dev at lists.llvm.org> wrote: > > 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). > > +1 for this. I think braces around single-line statements can be > allowed, but they really shouldn't be mandated, and that's been my > personal policy for reviews. In particular, > > if (!is_transform_applicable) { > return {}; > } > > is very aggravating clutter. > > Braces should be required around multi-line statements. Note: > > BAD: > for (...) > for (...) > single_line_statement; > > GOOD: > for (...) { > for (...) > single_line_statement; > } > > Cheers, > Nicolai > -- > Lerne, wie die Welt wirklich ist, > aber vergiss niemals, wie sie sein sollte. > _______________________________________________ > 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
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.Followed by the rest of the section that describes cases where readability or maintainability would be harmed. And then be done with it. - Steve On 6/22/20, 1:44 PM, "Chris Lattner" <clattner at nondot.org> wrote: External email: Use caution opening links or attachments For those who don’t like it, is the currently documented policy broken enough to be important to changing? I assume you wouldn’t recommend a massive rewrite of the codebase, so we’re going to be with this for quite some time. -Chris > On Jun 22, 2020, at 1:36 PM, Steve Scalpone via llvm-dev <llvm-dev at lists.llvm.org> wrote: > > Did this conversation reach a conclusion? > > My ad hoc tally says that a slight majority of the responders preferred to fully brace statements and no one wanted to totally eliminate braces. > > The technical arguments for fully braced statements were 1) it's considered a slightly safer coding style and 2) commit diffs with fully braced statements may be slightly more to the point. > > I didn't register any technical arguments for less-than-fully-braced statement -- the preference seemed to be aesthetic. I may have missed a technical argument. > > Certainly an "always use braces" rule would be simpler than what's documented now in the LLVM Coding Standards [1]. > > Another option would be to make braces a developer's choice, and ask that those omitting braces please follow the rules documented in [1]. > > [1] https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements > > On 6/18/20, 3:56 AM, "llvm-dev on behalf of Nicolai Hähnle via llvm-dev" <llvm-dev-bounces at lists.llvm.org on behalf of llvm-dev at lists.llvm.org> wrote: > > External email: Use caution opening links or attachments > > > On Tue, Jun 16, 2020 at 10:35 AM Momchil Velikov via llvm-dev > <llvm-dev at lists.llvm.org> wrote: >> 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). > > +1 for this. I think braces around single-line statements can be > allowed, but they really shouldn't be mandated, and that's been my > personal policy for reviews. In particular, > > if (!is_transform_applicable) { > return {}; > } > > is very aggravating clutter. > > Braces should be required around multi-line statements. Note: > > BAD: > for (...) > for (...) > single_line_statement; > > GOOD: > for (...) { > for (...) > single_line_statement; > } > > Cheers, > Nicolai > -- > Lerne, wie die Welt wirklich ist, > aber vergiss niemals, wie sie sein sollte. > _______________________________________________ > 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
On Mon, Jun 22, 2020 at 4:59 PM David Blaikie via llvm-dev < llvm-dev at lists.llvm.org> wrote:> Make for some > cognitive dissonance while reading code - you read a few lines written > one way, then switch, etc. >That's my top annoyance with the current style though: The cognitive dissonance of encountering unavoidable braces in between blocks of brace-free code. I don't mind the lack of braces in Python. The mix in the LLVM codebase is mind-bending to me. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200622/1c88f3a7/attachment.html>
While I personally would be in favor of the rule becoming "always use braces", I think it's hard to justify the amount of work it would take to realize that goal throughout the codebase. That said, I think it might be worth changing the rule from "Don’t Use Braces on Simple ..." to "Prefer Not to Use Braces on Simple ... " or "Braces May Optionally be Omitted on Simple ...". The old version mandates omitting braces based upon a judgement call. The new versions allow omitting braces based on a judgement call. Complicated rules that mandate specific things based upon a judgement call are very hard to enforce, and can be infuriating when a developer with more clout than you is blocking your work because their interpretation of the rules differs from your own. I'm not saying this has happened in my work on LLVM, but it has happened to me in my career. Thanks, Christopher Tetreault -----Original Message----- From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of Chris Lattner via llvm-dev Sent: Monday, June 22, 2020 1:45 PM To: Steve Scalpone <sscalpone at nvidia.com> Cc: llvm-dev at lists.llvm.org; Matt Arsenault <arsenm2 at gmail.com> Subject: [EXT] Re: [llvm-dev] Codifying our Brace rules- For those who don’t like it, is the currently documented policy broken enough to be important to changing? I assume you wouldn’t recommend a massive rewrite of the codebase, so we’re going to be with this for quite some time. -Chris> On Jun 22, 2020, at 1:36 PM, Steve Scalpone via llvm-dev <llvm-dev at lists.llvm.org> wrote: > > Did this conversation reach a conclusion? > > My ad hoc tally says that a slight majority of the responders preferred to fully brace statements and no one wanted to totally eliminate braces. > > The technical arguments for fully braced statements were 1) it's considered a slightly safer coding style and 2) commit diffs with fully braced statements may be slightly more to the point. > > I didn't register any technical arguments for less-than-fully-braced statement -- the preference seemed to be aesthetic. I may have missed a technical argument. > > Certainly an "always use braces" rule would be simpler than what's documented now in the LLVM Coding Standards [1]. > > Another option would be to make braces a developer's choice, and ask that those omitting braces please follow the rules documented in [1]. > > [1] > https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple- > single-statement-bodies-of-if-else-loop-statements > > On 6/18/20, 3:56 AM, "llvm-dev on behalf of Nicolai Hähnle via llvm-dev" <llvm-dev-bounces at lists.llvm.org on behalf of llvm-dev at lists.llvm.org> wrote: > > External email: Use caution opening links or attachments > > > On Tue, Jun 16, 2020 at 10:35 AM Momchil Velikov via llvm-dev > <llvm-dev at lists.llvm.org> wrote: >> 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). > > +1 for this. I think braces around single-line statements can be > allowed, but they really shouldn't be mandated, and that's been my > personal policy for reviews. In particular, > > if (!is_transform_applicable) { > return {}; > } > > is very aggravating clutter. > > Braces should be required around multi-line statements. Note: > > BAD: > for (...) > for (...) > single_line_statement; > > GOOD: > for (...) { > for (...) > single_line_statement; > } > > Cheers, > Nicolai > -- > Lerne, wie die Welt wirklich ist, > aber vergiss niemals, wie sie sein sollte. > _______________________________________________ > 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_______________________________________________ LLVM Developers mailing list llvm-dev at lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
I'll note that reading along I haven't found any of the proposed changes particularly worthwhile. I'm also not strongly opposed to any of them - I just don't care - but I certainly haven't been convinced there's any clear benefit to be had by changing our current policy. Philip On 6/22/20 1:44 PM, Chris Lattner via llvm-dev wrote:> For those who don’t like it, is the currently documented policy broken enough to be important to changing? > > I assume you wouldn’t recommend a massive rewrite of the codebase, so we’re going to be with this for quite some time. > > -Chris > >> On Jun 22, 2020, at 1:36 PM, Steve Scalpone via llvm-dev <llvm-dev at lists.llvm.org> wrote: >> >> Did this conversation reach a conclusion? >> >> My ad hoc tally says that a slight majority of the responders preferred to fully brace statements and no one wanted to totally eliminate braces. >> >> The technical arguments for fully braced statements were 1) it's considered a slightly safer coding style and 2) commit diffs with fully braced statements may be slightly more to the point. >> >> I didn't register any technical arguments for less-than-fully-braced statement -- the preference seemed to be aesthetic. I may have missed a technical argument. >> >> Certainly an "always use braces" rule would be simpler than what's documented now in the LLVM Coding Standards [1]. >> >> Another option would be to make braces a developer's choice, and ask that those omitting braces please follow the rules documented in [1]. >> >> [1] https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements >> >> On 6/18/20, 3:56 AM, "llvm-dev on behalf of Nicolai Hähnle via llvm-dev" <llvm-dev-bounces at lists.llvm.org on behalf of llvm-dev at lists.llvm.org> wrote: >> >> External email: Use caution opening links or attachments >> >> >> On Tue, Jun 16, 2020 at 10:35 AM Momchil Velikov via llvm-dev >> <llvm-dev at lists.llvm.org> wrote: >>> 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). >> +1 for this. I think braces around single-line statements can be >> allowed, but they really shouldn't be mandated, and that's been my >> personal policy for reviews. In particular, >> >> if (!is_transform_applicable) { >> return {}; >> } >> >> is very aggravating clutter. >> >> Braces should be required around multi-line statements. Note: >> >> BAD: >> for (...) >> for (...) >> single_line_statement; >> >> GOOD: >> for (...) { >> for (...) >> single_line_statement; >> } >> >> Cheers, >> Nicolai >> -- >> Lerne, wie die Welt wirklich ist, >> aber vergiss niemals, wie sie sein sollte. >> _______________________________________________ >> 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 > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Chris Lattner via llvm-dev <llvm-dev at lists.llvm.org> writes:> For those who don’t like it, is the currently documented policy broken > enough to be important to changing?I believe it is, simply for safety reasons. Adding a statement to a single-statement block can introduce bugs. Mandating braces everywhere prevents that.> I assume you wouldn’t recommend a massive rewrite of the codebase, so > we’re going to be with this for quite some time.Sure, that's unavoidable. But as code changes we can add braces where they're missing and reviews should catch cases where we forget. -David