[llvm-dev] Codifying our Brace rules-

Mehdi AMINI via llvm-dev llvm-dev at lists.llvm.org
Fri Jun 26 10:50:39 PDT 2020 

On Fri, Jun 26, 2020 at 9:31 AM Adrian McCarthy <amccarth at google.com> wrote:

> 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.
>

You're right. To clarify: I didn't mean to promote one style or the other,
I only included it here as an illustration, for the sake of the discussion.
I produced them originally just as a "test" to see for myself the actual
impact of always adding braces.




>
> 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):
>>
>> <attachment>
>>  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/52e083c8/attachment.html>

More information about the llvm-dev mailing list