Enum formatting changes - polly automatic formatting verifier

[Daniel Jasper]

On Sun, Jan 5, 2014 at 11:31 PM, Tobias Grosser <tobias at grosser.es> wrote:

> On 01/05/2014 09:44 PM, Daniel Jasper wrote:
>
>> On Sun, Jan 5, 2014 at 3:58 PM, Tobias Grosser <tobias at grosser.es> wrote:
>>
>>  On 01/05/2014 02:13 PM, Daniel Jasper wrote:
>>>
>>>  Tobias, could you submit the two small formatting changes that are
>>>> required? I am not entirely certain whether it would be ok for me to
>>>> just
>>>> submit patches to polly. Also I don't think it would be a good idea for
>>>> clang-format developers to be required to ensure other projects'
>>>> adherence
>>>> to what clang-format deems to be correct style.
>>>>
>>>>
>>> Hi Daniel,
>>>
>>> thanks for letting me know. You are not required to fix Polly, but if
>>> there is an obvious and easy fix, I would be more than happy if you do so
>>> (it also keeps the buildbot noise down). For non-obvious changes I would
>>> prefer a short pre-commit mail (or just an headsup as today). However, if
>>> the style change is non-obvious it would be great if you could make it
>>> off
>>> by default for LLVM and give a short headsup on the mailing list such
>>> that
>>> I can prepare a patch for Polly.
>>>
>>> Regarding the change today, I wonder how you figured out that the new
>>> formatting is 'the predominant choice' in LLVM/clang? I just checked what
>>> formatting is more common in LLVM/clang by counting the one line enums in
>>> the current LLVM and clang code and after running the latest clang-format
>>> over all .cpp and .h files.
>>>
>>> I use the following grep call to count:
>>>
>>> grep -R 'enum.*{.*}.*;' tools/clang/lib/ lib/ include/ \
>>>                          tools/clang/include | wc
>>>
>>> I get 259 cases for the current formatting and 531 for the reformatted
>>> code, which means 272 cases use currently a multi line formatting.
>>>
>>>
>> I did a slightly different analysis. I searched for "enum.*}" and there
>> seem to be 432 files with in LLVM/Clang containing these. This compares to
>> a search (with multiline regexs) for "\n\ *enum[^\}]{0,73}\n\}" of which I
>> find 80 files containing them. The latter should be all enums that can be
>> written on a single line but aren't.
>>
>> Without paths containing "test" these numbers change to 219 vs. 47 files.
>> The 5:1 ratio seems quite consistent. The regular expressions might have
>> errors, I only hacked at them until I was satisfied and the individual
>> findings were what I was looking for.
>>
>
> It seems there slipped in a bigger problem in your second regexp. You do
> not count any indented multiline enums as you match for '\n\}' at the end.
> (Also fixing your regexp is not that easy as multi-line enums with more
> than 73 characters can easily fit into a line as most of the whitespace
> used for indentation can be removed).
>
> Do you see any problems in using clang-format directly as proposed above?
> Or can we agree that there is no single line enums are not more common?


Chandler has already addressed most of the concerns better than I could.
clang-format (being half-way useful) is still less than a year old and we
need to be able to make some changes or else development will become
unnecessarily hard.

As for this specific change, I tried to explain the numbers I used.
Probably your numbers are more accurate and a better way to analyze the
current state. However, even based on your numbers, it is an even split.
You find 259 cases in favor of single line formatting and 272 cases in
favor of one enumerator per line. That is as close to a break-even as it
gets. Thus, before and after the change, clang-format would change about
the same amount of code of the LLVM codebase making the change a neutral
one. Adding the preference of users, e.g. expressed through llvm.org/PR16517,
this still makes a good change.

 It seems the one line formatting is actually slightly less common in the
>>
>>> LLVM/clang files. I also don't believe that the new style buys us
>>> anything
>>> in readability.
>>>
>>>
>> It still makes sense to build clang-format close to the more frequent
>> style.
>>
>
> Agreed. Unfortunately it seems this is not the case here.
>
>
>  Plus, the people I talked to before-hand (Manuel, Alex and a few
>> other users prefer single lines).
>>
>
> I did not see this discussion. Though I propose if you meet internally and
> agree on a new or more precise style for LLVM, the next step should
> probably be to submit a patch to the official style guide for review. If it
> is accepted, changing the LLVM style in clang-format is a no-brainer.
>
>
>  I would prefer clang-format to remain consistent between versions,
>>
>>> as long as there is no good reason to break this consistency.
>>>
>>
>>
>> I think the above (preferences of users + more similarity with existing
>> code) constitutes a good reason.
>>
>
> See above.
>
>
>  Consequently, I wonder if it would not make sense to use the old style in
>>> LLVM mode?
>>>
>>>
>> a) Not from the numbers I had. We can do a more thorough analysis, though.
>>
>
> They seem incorrect. The analysis I reported seems to give reliable
> numbers.
>
>
>  b) If there is an even split inside LLVM (as your numbers would suggest)
>> then I'd prefer to have fewer options.
>>
>
> What is the problem with additional options? I did not touch them once yet.
>

Every option has additional maintenance costs and makes it harder for users
to discover the options they really want to set.

These costs are not huge, but IMO they tip the balance if LLVM is otherwise
evenly split.

However, as people start to use clang-format regularly we will start to
> have problems with different versions of clang-format formatting
> differently. (This does not affect companies who always provide the latest
> version to their developers. But this may not be the common case).
>
> In the last months clang-format has been shown to be stable and consistent
> with changes only affecting rare special cases. This change impacts over
> 500 cases. Being here inconsistent without a very good reason would be a
> pity.
>
>
>  Btw. this is not only Google Style. GNU style has this explicitly written
>> down, in WebKit it seems to be common.
>>
>
> I mostly care about consistency within the LLVM code base.
>
> Cheers,
> Tobias
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140106/4efb935d/attachment.html>