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"? Jay.
> -----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
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>
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