[llvm-dev] Codifying our Brace rules-
Mehdi AMINI via llvm-dev
llvm-dev at lists.llvm.org
Thu Jun 25 14:50:50 PDT 2020
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/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>
More information about the llvm-dev
mailing list