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

[Daniel Jasper]

Somehow cfe-commits got lost.. Adding that back to CC..


On Thu, Oct 24, 2013 at 9:41 AM, Daniel Jasper <djasper at google.com> wrote:

> Yes you are right, I was confused. However, my confusion was stemming from
> the fact that your tests don't include the interesting case:
>
> +TEST_F(FormatTest, BreaksBeforeClassInheritanceComma) {
> +  FormatStyle Style = getWebKitStyle();
> +  Style.BreakClassInheritanceListsBeforeComma = true;
> +
> +  verifyFormat("namespace outer {\n"
> +               "int i;\n"
> +               "namespace inner {\n"
> +               "    int i;\n"
> +               "    class Aaaa : public Bbb\n"
> +               "               , protected Ccc {\n"
>
> I would be interesting to see what happens if you actually have to break
> before "public" (i.e. because you'd otherwise violate the column limit).
>
> Come to think of that, is this patch really doing what it is supposed to
> do? Can you specify what exactly BreakClassInheritanceListsBeforeColon is
> supposed to do? The options are:
> 1) Always break before the colon
> 2) Break before the colon if the entire line (up to "{") doesn't fit into
> the column limit
> 3) Break before the colon if the first base class (i.e. everything up to
> ",") does not fit on one line
> 4) Break before the colon if the first base class does not fit on one line
> or one of the following base classes cannot be aligned nicely
>
> I would intuitively think that option #4 is what you are trying to
> implement (based on configuration flag's comment). Maybe we should state
> this more clearly and add more tests.
>
> If you are going to write more tests, feel free to make them shorter.
> Leave out the namespaces and global variables. And maybe move the
> constructor with initializers to its own verifyFormat(). We only need to
> show once (for each config option) that it does not influence constructor
> initializers.
>
> Cheers and sorry for not spotting this earlier!
> Daniel
>
>
> On Thu, Oct 24, 2013 at 9:24 AM, Curdeius Curdeius <curdeius at gmail.com>wrote:
>
>> Hi Daniel,
>> thanks again.
>> Just a small remark.
>> You say I'm not testing all the 4 possible cases, but (in my opinion), I
>> do.
>> I created 3 tests:
>>
>>    - TEST_F(FormatTest, BreaksBeforeClassInheritanceColonAndComma)  -
>>    true, true case
>>    - TEST_F(FormatTest, BreaksBeforeClassInheritanceComma) - false, true
>>    case
>>    - TEST_F(FormatTest, BreaksBeforeClassInheritanceColon) = true, false
>>    case
>>
>> The 4th case (false, false) is taken into account by existing tests.
>>
>> I'll take into account and write back ASAP.
>>
>> Cheers,
>> Marek
>>
>>
>> 2013/10/23 Daniel Jasper <djasper at google.com>
>>
>>>
>>>
>>>
>>> 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/20131024/814b2bc4/attachment.html>