[PATCH] [clang-extra-tools] [clang-format]

[Daniel Jasper]

On Wed, Oct 23, 2013 at 3:44 PM, Curdeius Curdeius <curdeius at gmail.com>wrote:

> Hi Daniel,
> thanks for your feedback and sorry for the late response.
>
> I attach the patch.
>
> My comments to your remarks below.
>
> * How is the new test in FormatsWithWebKitStyle related?
>>
>
> I removed the unnecessary mess.
>
>
>> * Either collapse both parameters into a single parameter or add test
>> cases for all combinations.
>>
>
> I did so.
>

Not really. You are still not testing ..BeforeColon = false and
..BeforeComma = true. I agree that it does not make much sense, but then,
an enum containing exactly the three configuration values seems to be a
better solution. You can take a look at UseTabStyle for an example of how
to do that. I imagine an enum like:

enum InheritanceBreakingStyle {
  IBS_AfterColon,
  IBS_BeforeColon,
  IBS_BeforeComma
};

Does that make sense?


>
>>  * The parameters themselves have tests (ParsesConfiguration).
>>
>
> Ok, done.
>
>
>>
>> +    case tok::semi:
>>> +      Contexts.back().CanBeInInheritanceList = false;
>>> +      break;
>>
>>
>> Is this necessary? Can you create a test that breaks without it?
>>
>
> You're right. It wasn't necessary
>
>
>>
>>      case tok::l_brace:
>>> +      Contexts.back().CanBeInInheritanceList = false;
>>>        if (!parseBrace())
>>
>>
>> Is this necessary? Can you create a test that breaks without it?
>>
>
> You're right again. It wasn't necessary.
>
>
>>
>>  +        //} else if (Contexts.size() == 1) {
>>> +        // FIXME: annotates switch-case colon as inheritance colon
>>> +        // FIXME: annotates access specifiers as inheritance colon
>>> +        // Tok->Type = TT_OtherColon;
>>
>>
>> Is this intentional?
>>
>
> I removed this part.
>
>
>>
>>        else if (Previous.Type == TT_InheritanceColon)
>>
>> -        State.Stack.back().Indent = State.Column;
>>> +        // We had a colon on a newline. Now, if we want to align commas
>>> to the
>>> +        // colon, we indent less by -2 == length(", ")
>>
>> +        if (Style.BreakClassInheritanceListsBeforeComma)
>>> +          State.Stack.back().Indent = State.Column - 2;
>>> +        else
>>> +          State.Stack.back().Indent = State.Column;
>>
>>
>> Remove this completely.
>>
>
> Well, I couldn't remove it completely.
> If you have any idea how to do it in a cleaner way, I'll be glad to know.
>

Yes, you could have. By now, there is one more statement (changing
LastSpace), but everything that was in there could have been removed. As
this is getting too complicated to explain, I have attached a patch
removing everything that I think can be removed (and the tests still pass).
Also, I have cleaned up a few basic formatting errors. Interestingly, you
don't seem to be using clang-format ;-).


>> +    if (Current.Type == TT_InheritanceColon) {
>>>        State.Stack.back().AvoidBinPacking = true;
>>> +      if (!Style.BreakClassInheritanceListsBeforeComma)
>>> +        State.Stack.back().Indent = State.Column + 2;
>>> +    }
>>
>>
>> Change this to
>>
>> if (Current.Type == TT_InheritanceColon) {
>>   State.Stack.back().AvoidBinPacking = true;
>>   State.Stack.back().Indent = State.Column;
>>   if (!Style.BreakClassInheritanceListsBeforeComma)
>>     State.Stack.back().Indent += 2;
>> }
>>
>
> Done.
>
>
>>  On Fri, Aug 2, 2013 at 10:59 AM, Curdeius Curdeius <curdeius at gmail.com>wrote:
>>
>>>  Hi,
>>>
>>> I've added two options to clang-format:
>>> * BreakClassInheritanceListsBeforeColon
>>> and
>>> * BreakClassInheritanceListsBeforeComma.
>>>
>>> They allow us to break on class inheritance lists similarly to
>>> constructor initializer lists.
>>>
>>> I've corrected as well the TokenAnnotator, which mistakenly annotated
>>> colons from switch statements ("case:") and from access modifiers
>>> ("public:") as TT_InheritanceColon.
>>>
>>> The patch in the attachment.
>>>
>>> Cheers,
>>> Marek Kurdej
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>>
>>
>
> Regards,
> Marek Kurdej
>

+  } else if (Current.Type == TT_InheritanceComma) {
+    if (Style.BreakClassInheritanceListsBeforeComma)
+      State.Stack.back().Indent -= 2; // 2 is length of ", "
+  }

I also removed this as it seemed suspicious .. Tests still pass :-).

Please start from the attached patch when continuing to work on this.

Cheers,
Daniel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131023/17212082/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-format-inheritance-lists.patch
Type: text/x-patch
Size: 11979 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131023/17212082/attachment.bin>