On Wed, Jun 24, 2020 at 2:37 AM Chris Lattner via llvm-dev < llvm-dev at lists.llvm.org> wrote:> On Jun 23, 2020, at 11:02 AM, Philip Reames <listmail at philipreames.com> > wrote: > > 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. > > I agree. The discussion is also hard to follow, because there are many > different competing suggestions and opinions. There are a couple of people > talking about clarifying the rules to be less prescriptive, which seem like > it is worth discussing."Clarifying the rules to be less prescriptive" sounds self-contradictory. Are you in favor of talking about clarifying the existing guidelines or changing them to be less prescriptive? Or maybe you want to change them a little so that they are easier to express clearly? There are already several well-defined de facto standard brace styles. One way to make the guidelines clear (and concise) is simply to declare LLVM uses $(FOO) Brace Style with a link to the Wikipedia description. That suggests to me that it's not super feasible to divorce clarification from style choice, at least, not without putting a bound on how clear we can be.> I think we should take the suggestion of “always require braces” off the > table, because it doesn’t make sense given the impact to the code base. >Given that the codebase is already riddled with inconsistencies (and instances that I cannot determine the correctness against the current guidelines), I don't understand why you think it doesn't make sense to consider a simpler scheme. The current inconsistencies exist because the rules are unclear and, because of the edge cases, hard to internalize. A simpler rule (or set of rules) would presumably result in fewer inconsistencies going forward, so the code would evolve toward a more consistent state.> > -Chris > > > > > 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 > > _______________________________________________ > 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/323abf9e/attachment.html>
On Wed, Jun 24, 2020 at 11:51 AM Adrian McCarthy via llvm-dev < llvm-dev at lists.llvm.org> wrote:> > > On Wed, Jun 24, 2020 at 2:37 AM Chris Lattner via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > >> On Jun 23, 2020, at 11:02 AM, Philip Reames <listmail at philipreames.com> >> wrote: >> > 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. >> >> I agree. The discussion is also hard to follow, because there are many >> different competing suggestions and opinions. There are a couple of people >> talking about clarifying the rules to be less prescriptive, which seem like >> it is worth discussing. > > > "Clarifying the rules to be less prescriptive" sounds self-contradictory. > Are you in favor of talking about clarifying the existing guidelines or > changing them to be less prescriptive? Or maybe you want to change them a > little so that they are easier to express clearly? > > There are already several well-defined de facto standard brace styles. > One way to make the guidelines clear (and concise) is simply to declare > LLVM uses $(FOO) Brace Style with a link to the Wikipedia description. > That suggests to me that it's not super feasible to divorce clarification > from style choice, at least, not without putting a bound on how clear we > can be. > > >> I think we should take the suggestion of “always require braces” off >> the table, because it doesn’t make sense given the impact to the code base. >> > > Given that the codebase is already riddled with inconsistencies (and > instances that I cannot determine the correctness against the current > guidelines), I don't understand why you think it doesn't make sense to > consider a simpler scheme. The current inconsistencies exist because the > rules are unclear and, because of the edge cases, hard to internalize. A > simpler rule (or set of rules) would presumably result in fewer > inconsistencies going forward, so the code would evolve toward a more > consistent state. >Can you clarify what is unclear with the current rule <https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements> ? The title of the section is "Don't Use Braces on Simple Single-Statement Bodies of if/else/loop Statements", which seems already fairly clear. It then also mentions exceptions to the rule: readability and maintainability ; and clarifies what is considered not readable and not maintainable. It even gives two examples. Maybe adding more examples there could help? -- Mehdi> > >> >> -Chris >> >> > >> > 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 >> >> _______________________________________________ >> 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/20200624/c99387fa/attachment.html>
On Wed, Jun 24, 2020 at 5:35 PM Mehdi AMINI <joker.eph at gmail.com> wrote:> > > On Wed, Jun 24, 2020 at 11:51 AM Adrian McCarthy via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > >> >> >> On Wed, Jun 24, 2020 at 2:37 AM Chris Lattner via llvm-dev < >> llvm-dev at lists.llvm.org> wrote: >> >>> On Jun 23, 2020, at 11:02 AM, Philip Reames <listmail at philipreames.com> >>> wrote: >>> > 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. >>> >>> I agree. The discussion is also hard to follow, because there are many >>> different competing suggestions and opinions. There are a couple of people >>> talking about clarifying the rules to be less prescriptive, which seem like >>> it is worth discussing. >> >> >> "Clarifying the rules to be less prescriptive" sounds >> self-contradictory. Are you in favor of talking about clarifying the >> existing guidelines or changing them to be less prescriptive? Or maybe you >> want to change them a little so that they are easier to express clearly? >> >> There are already several well-defined de facto standard brace styles. >> One way to make the guidelines clear (and concise) is simply to declare >> LLVM uses $(FOO) Brace Style with a link to the Wikipedia description. >> That suggests to me that it's not super feasible to divorce clarification >> from style choice, at least, not without putting a bound on how clear we >> can be. >> >> >>> I think we should take the suggestion of “always require braces” off >>> the table, because it doesn’t make sense given the impact to the code base. >>> >> >> Given that the codebase is already riddled with inconsistencies (and >> instances that I cannot determine the correctness against the current >> guidelines), I don't understand why you think it doesn't make sense to >> consider a simpler scheme. The current inconsistencies exist because the >> rules are unclear and, because of the edge cases, hard to internalize. A >> simpler rule (or set of rules) would presumably result in fewer >> inconsistencies going forward, so the code would evolve toward a more >> consistent state. >> > > Can you clarify what is unclear with the current rule > <https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements> > ? >I'm not the one who asserted that the goal of this thread is to clarify the rule. But since you asked, I don't understand the sentences about hoisted comments. Readability is harmed when a single statement is accompanied by a comment> that loses its meaning if hoisted above the if or loop statement.Does this mean "a comment that *would lose* its meaning if *it were to be* hoisted"?> Note that comments should only be hoisted for loops and if, and not in > else if or else, where it would be unclear whether the comment belonged > to the preceeding condition, or the else. >This seems to imply that we should be hoisting comments up whenever we can. Is that actually the case? Or is this simply guidance for when someone is considering hoisting a comment? In any event, I'm not the one who said we should clarify the rule. In fact, I was trying to get clarification about Chris's comment about both clarifying and/or changing the rule. It's my opinion that we should simplify the rule. My issues are: - The rule is overly complex for what it accomplishes. There are a lot of details and edge cases adding to the cognitive load when trying to read and/or write the code. I believe that's the root cause of many of the misunderstandings and of the existing inconsistencies in the code. - As currently worded, the rule contradicts claims I've seen in code reviews and up-thread. - For example, I've seen requests to add braces to both the if and the else blocks when only one of those requires it. If I read the current text of the rule, that _might_ be covered by "This list is not exhaustive, for example, readability is also harmed if an if/else chain starts using braced bodies partway through and does not continue on with braced bodies." On the other hand that wording seems to say you shouldn't go up and add braces to the if body just because the else body requires it, a position reinforced in the example in the guidelines, but you should consistently add them below the first use. - Another example is whether you should add braces when the body is a single statement that takes multiple lines. In the present wording, as I understand it, that's the case *only* if one or more of the lines is a comment (and even then, only in certain circumstances?!), but not if the statement itself wraps across multiple lines. - Even if the entire codebase followed the rules consistently, when combined with other style decisions, the fact that there are sometimes braces and sometimes not inhibits visual pattern matching when reading the code, at least for some, including myself. It wouldn't be as bad if we had more generous indentation levels and if we didn't grant indentation exceptions for case labels and public/protected/private markers in class definitions. It's easy to find examples to illustrate that the code is not currently even close to consistent; the code often uses braces where the guidelines tell us not to. And there's no real harm done by that, not for readability, not for maintenance, and not even for consistency since we're already in an inconsistent state. A simpler rule with fewer exceptions and less need for examples would be a benefit. I spent a few minutes scrolling through Support/Path.cpp and found these illustrative examples: https://github.com/llvm/llvm-project/blob/master/llvm/lib/Support/Path.cpp#L353 if (has_net || has_drive) { if ((++pos != e) && is_separator((*pos)[0], style)) { // {C:/,//net/}, so get the first two components. return path.substr(0, b->size() + pos->size()); } else { // just {C:,//net}, return the first component. return *b; } } I believe, according to the rule, the outer `if` should not have braces because its body is a single statement and there's no risk of a dangling `else`. On the other hand, there's the "this list is not exhaustive" escape clause which opens the door to subjectivity. (Personally, I think the braces here do help readability, but that doesn't seem to be the predominant opinion in the community.) I believe the blocks on the inner `if` are correctly braced because the bodies also have comments. I'll admit I don't understand all the wording about hoisting comments, but I'm pretty sure that doesn't apply here. https://github.com/llvm/llvm-project/blob/master/llvm/lib/Support/Path.cpp#L363 // POSIX style root directory if (is_separator((*b)[0], style)) { return *b; } There are lots of examples of this kind of inconsistency. But maybe this started out with braces because the comment was inside the body, and then it was later hoisted above the `if` (where it makes less sense) and the hoister forgot to remove the braces. https://github.com/llvm/llvm-project/blob/master/llvm/lib/Support/Path.cpp#L396 if ((has_net || has_drive) && // {C:,//net}, skip to the next component. (++pos != e) && is_separator((*pos)[0], style)) { return *pos; } According to the rule, the braces don't belong here, so we could just treat this as another example of general inconsistency. But, oh my. That's a line in the middle of the conditional expression. Was that a merge error? Was that a cut+paste error while trying to hoist (or sink?) the comment? Or is the comment actually intended to describe just what the first part of the expression helps us determine. I'm thankful for the braces here--they help this unusual pattern stand out. https://github.com/llvm/llvm-project/blob/master/llvm/lib/Support/Path.cpp#L733 // Check for path traversal components or double separators. if (component.empty() || component == ".") { needs_change = true; } else if (remove_dot_dot && component == "..") { needs_change = true; // Do not allow ".." to remove the root component. If this is the // beginning of a relative path, keep the ".." component. if (!components.empty() && components.back() != "..") { components.pop_back(); } else if (!absolute) { components.push_back(component); } } else { components.push_back(component); } I think, if we spend a minute or two studying this with the guidelines in an adjacent window, we can agree that there are exactly three sets of braces that should not be there. I argue that code authors and reviewers should not have to spend a minute or two. A simpler rule would save that cognitive load for more important details. As a reader, I appreciate the consistent use of braces here rather than what it would have looked like without them.>The title of the section is "Don't Use Braces on Simple Single-Statement> Bodies of if/else/loop Statements", which seems already fairly clear. > It then also mentions exceptions to the rule: readability and > maintainability ; and clarifies what is considered not readable and not > maintainable. It even gives two examples. >> Maybe adding more examples there could help? >--> Mehdi > > > > > > >> >> >>> >>> -Chris >>> >>> > >>> > 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 >>> >>> _______________________________________________ >>> 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/20200625/87df6e01/attachment-0001.html>
On Thu, Jun 25, 2020 at 2:14 PM Mehdi AMINI <joker.eph at gmail.com> wrote:> > > On Wed, Jun 24, 2020 at 5:35 PM Mehdi AMINI <joker.eph at gmail.com> wrote: > >> >> >> On Wed, Jun 24, 2020 at 11:51 AM Adrian McCarthy via llvm-dev < >> llvm-dev at lists.llvm.org> wrote: >> >>> >>> >>> On Wed, Jun 24, 2020 at 2:37 AM Chris Lattner via llvm-dev < >>> llvm-dev at lists.llvm.org> wrote: >>> >>>> On Jun 23, 2020, at 11:02 AM, Philip Reames <listmail at philipreames.com> >>>> wrote: >>>> > 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. >>>> >>>> I agree. The discussion is also hard to follow, because there are many >>>> different competing suggestions and opinions. There are a couple of people >>>> talking about clarifying the rules to be less prescriptive, which seem like >>>> it is worth discussing. >>> >>> >>> "Clarifying the rules to be less prescriptive" sounds >>> self-contradictory. Are you in favor of talking about clarifying the >>> existing guidelines or changing them to be less prescriptive? Or maybe you >>> want to change them a little so that they are easier to express clearly? >>> >>> There are already several well-defined de facto standard brace styles. >>> One way to make the guidelines clear (and concise) is simply to declare >>> LLVM uses $(FOO) Brace Style with a link to the Wikipedia description. >>> That suggests to me that it's not super feasible to divorce clarification >>> from style choice, at least, not without putting a bound on how clear we >>> can be. >>> >>> >>>> I think we should take the suggestion of “always require braces” off >>>> the table, because it doesn’t make sense given the impact to the code base. >>>> >>> >>> Given that the codebase is already riddled with inconsistencies (and >>> instances that I cannot determine the correctness against the current >>> guidelines), I don't understand why you think it doesn't make sense to >>> consider a simpler scheme. The current inconsistencies exist because the >>> rules are unclear and, because of the edge cases, hard to internalize. A >>> simpler rule (or set of rules) would presumably result in fewer >>> inconsistencies going forward, so the code would evolve toward a more >>> consistent state. >>> >> >> Can you clarify what is unclear with the current rule >> <https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements> >> ? >> The title of the section is "Don't Use Braces on Simple Single-Statement >> Bodies of if/else/loop Statements", which seems already fairly clear. >> It then also mentions exceptions to the rule: readability and >> maintainability ; and clarifies what is considered not readable and not >> maintainable. It even gives two examples. >> >> Maybe adding more examples there could help? >> > > Gave it a try here: https://reviews.llvm.org/D82594I'm fairly sure we can keep the spirit and write a clang-tidy rule that would cover most of the cases. I also ran an experiment adding all braces on one file: https://reviews.llvm.org/D82599 And to be more representative, here is a visual diff (low quality for the mailing-list) of what it looks like in an editor for a single function (left is full braces, right is currently committed): [image: Screen Shot 2020-06-25 at 2.11.29 PM.jpg] Best, -- Mehdi> >>>> > 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 >>>> >>>> _______________________________________________ >>>> 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/20200625/fda6c612/attachment-0001.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: Screen Shot 2020-06-25 at 2.11.29 PM.jpg Type: image/jpeg Size: 96946 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200625/fda6c612/attachment-0001.jpg>
Thanks. Those latest tweaks about comment hoisting does clarify for me. I'm not sure whether the code diffs are meant to promote one style above the other. They are different styles that make different tradeoffs over different features. Which one somebody prefers depends will depend on how they value those features. It's nice to have some tangible examples to compare though, so thanks for providing them. On Thu, Jun 25, 2020 at 2:51 PM Mehdi AMINI <joker.eph at gmail.com> wrote:> > On Thu, Jun 25, 2020 at 2:14 PM Mehdi AMINI <joker.eph at gmail.com> wrote: > >> >> >> On Wed, Jun 24, 2020 at 5:35 PM Mehdi AMINI <joker.eph at gmail.com> wrote: >> >>> >>> >>> On Wed, Jun 24, 2020 at 11:51 AM Adrian McCarthy via llvm-dev < >>> llvm-dev at lists.llvm.org> wrote: >>> >>>> >>>> >>>> On Wed, Jun 24, 2020 at 2:37 AM Chris Lattner via llvm-dev < >>>> llvm-dev at lists.llvm.org> wrote: >>>> >>>>> On Jun 23, 2020, at 11:02 AM, Philip Reames <listmail at philipreames.com> >>>>> wrote: >>>>> > 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. >>>>> >>>>> I agree. The discussion is also hard to follow, because there are >>>>> many different competing suggestions and opinions. There are a couple of >>>>> people talking about clarifying the rules to be less prescriptive, which >>>>> seem like it is worth discussing. >>>> >>>> >>>> "Clarifying the rules to be less prescriptive" sounds >>>> self-contradictory. Are you in favor of talking about clarifying the >>>> existing guidelines or changing them to be less prescriptive? Or maybe you >>>> want to change them a little so that they are easier to express clearly? >>>> >>>> There are already several well-defined de facto standard brace styles. >>>> One way to make the guidelines clear (and concise) is simply to declare >>>> LLVM uses $(FOO) Brace Style with a link to the Wikipedia description. >>>> That suggests to me that it's not super feasible to divorce clarification >>>> from style choice, at least, not without putting a bound on how clear we >>>> can be. >>>> >>>> >>>>> I think we should take the suggestion of “always require braces” off >>>>> the table, because it doesn’t make sense given the impact to the code base. >>>>> >>>> >>>> Given that the codebase is already riddled with inconsistencies (and >>>> instances that I cannot determine the correctness against the current >>>> guidelines), I don't understand why you think it doesn't make sense to >>>> consider a simpler scheme. The current inconsistencies exist because the >>>> rules are unclear and, because of the edge cases, hard to internalize. A >>>> simpler rule (or set of rules) would presumably result in fewer >>>> inconsistencies going forward, so the code would evolve toward a more >>>> consistent state. >>>> >>> >>> Can you clarify what is unclear with the current rule >>> <https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements> >>> ? >>> The title of the section is "Don't Use Braces on Simple Single-Statement >>> Bodies of if/else/loop Statements", which seems already fairly clear. >>> It then also mentions exceptions to the rule: readability and >>> maintainability ; and clarifies what is considered not readable and not >>> maintainable. It even gives two examples. >>> >>> Maybe adding more examples there could help? >>> >> >> Gave it a try here: https://reviews.llvm.org/D82594 > > I'm fairly sure we can keep the spirit and write a clang-tidy rule that > would cover most of the cases. > > I also ran an experiment adding all braces on one file: > https://reviews.llvm.org/D82599 > > And to be more representative, here is a visual diff (low quality for the > mailing-list) of what it looks like in an editor for a single function > (left is full braces, right is currently committed): > > [image: Screen Shot 2020-06-25 at 2.11.29 PM.jpg] > > Best, > > -- > Mehdi > > > > >>>>> > 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 >>>>> >>>>> _______________________________________________ >>>>> 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/20200626/dd09699a/attachment-0001.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: Screen Shot 2020-06-25 at 2.11.29 PM.jpg Type: image/jpeg Size: 96946 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200626/dd09699a/attachment-0001.jpg>