r212591 - clang-format: Revamp function declaration/definition indentation.

[Tobias Grosser]

On 09/07/2014 11:42, Chandler Carruth wrote:
> On Wed, Jul 9, 2014 at 2:18 AM, Tobias Grosser <tobias at grosser.es> wrote:
>
>> On 09/07/2014 11:01, Chandler Carruth wrote:
>>
>>> I significantly prefer the new formatting FWIW.
>>>
>>> I don't like continuations without indent at all. I'm willing to concede
>>> the necessity of them for functions, but for variables it feels quite bad
>>> to me.
>>>
>>> An example of where the old behavior was very confusing for me, let's take
>>> the following code:
>>>
>>> static llvm::cl::opt<whatever> MyLongOptionName(args, more_args, "yadda
>>> yadda");
>>>
>>> If we end up (due to the specific balance of string length above) breaking
>>> before "MyLongOptionName" we get no indent. But if we break after the "("
>>> we get 4 spaces of indent. This is even weirder if your variable doesn't
>>> use a "type name(arg, arg);" style of initialization, but instead is "type
>>> name = a + b;" as now 'name' would get no indent but 'a' as the start of
>>> the second line would get indent. I understand the C++ grammar rules which
>>> lead to that scenario, but find it really awkward when reading code.
>>>
>>> With the new logic, there is one rule for all indentation when breaking a
>>> variable declaration across multiple lines: 4 spaces of indent. There is
>>> also one rule for indentation when breaking after the return type of a
>>> function: no indent.
>>>
>>
>> I can follow your points and also disliked clang-format inconsistently
>> breaking lines in several cases.  So seeing work in this area is great.
>>
>> If just seemed the commit affects large portions of LLVM, actively try to
>> establish a certain coding style and change existing clang-format behavior
>> in many cases.
>>
>> For such kind of changes, I would prefer an addition to the LLVM coding
>> style that explains the reasoning and that ensures that we do once and for
>> all agree on a certain style. In this commit we should have the style
>> discussion.
>
>
> I think this is way too heavy weight of a process ... =[ It seems to slow
> everything down.

For 99% of the formatting changes, this is not needed at all. In fact, 
this was the largest clang-format change since over a year.

> I filed the bug specifically complaining about clang-format's changes (such
> as the revisions you cite) and none have argued for clang-format's current
> behavior. I know bugs aren't great points of discussion, but unless a bunch
> of people show up on this thread saying that they strongly prefer the other
> formatting, why not just fix polly and move on? I want to use, dogfood, and
> provide feedback on the new behavior, and I can't if we just revert the
> change because we're still pondering deeply the really marginal style
> implications here.

As only the two of you read the bug, I doubt anybody has seen this. (I 
at least did not see this besides me being interested).

This is not about fixing Polly or our clang-format buildbot (I just 
adjusted them).

This is about establishing a consensus in an area where people regularly 
commit code that disagrees with clang-format 
(lib/Target/AArch64/AArch64TargetMachine.cpp).

Also, in http://llvm.org/PR16157 the very same bug was "solved" already 
and Manual especially introduced the option that you just removed to 
address the cl::opt changes that now show up. Here is another open bug 
that talks about inconsistent formatting between different lines: 
http://llvm.org/PR15169

Formatting is mostly about taking decisions and sticking to them, so I 
think having a discussion once might avoid bike-shedding and 
clang-format noise afterwards.

Cheers,
Tobias