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. > > --paulrI don't have a strong opinion on exactly what rules we choose. I've gotten used to the style of omitting braces for single-statement bodies, and at least for me, this generally improves my comprehension of the code because it allows me to see more of it at once. I agree with Paul, however, that we should have rules (and, like all rules, they'll appear suboptimal in some circumstances). That's better than having the style determined by some fairly-arbitrary combined aesthetic sense of each patch author and its set of reviewers. -Hal> >> 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-- Hal Finkel Lead, Compiler Technology and Programming Languages Leadership Computing Facility Argonne National Laboratory
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. >> >>--paulrI 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 }>I don't have a strong opinion on exactly what rules we choose. I've >gotten used to the style of omitting braces for single-statement >bodies, and at least for me, this generally improves my comprehension >of the code because it allows me to see more of it at once. I agree >with Paul, however, that we should have rules (and, like all rules, >they'll appear suboptimal in some circumstances). That's better than >having the style determined by some fairly-arbitrary combined >aesthetic sense of each patch author and its set of reviewers. > > -Hal > > >> >>>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 > >-- >Hal Finkel >Lead, Compiler Technology and Programming Languages >Leadership Computing Facility >Argonne National Laboratory > >_______________________________________________ >LLVM Developers mailing list >llvm-dev at lists.llvm.org >https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Alexander Richardson via llvm-dev
2020-Jun-24 09:54 UTC
[llvm-dev] Codifying our Brace rules-
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.