r198924 - clang-format: Slightly adapt line break in edge case.

[Daniel Jasper]

On Sat, Jan 11, 2014 at 8:43 PM, dblaikie at gmail.com <dblaikie at gmail.com>wrote:

> I believe builders already have a special case - not sure if this would
> affect those
>
> Daniel - got a diff of what changes in LLVM between a format of the whole
> project with/without this change?
>
> On Sat Jan 11 2014 at 11:39:43 AM, Eric Christopher <echristo at gmail.com>
> wrote:
>
>> There's a lot of stuff that looks like this:
>>
>>       MIB = BuildMI(mainMBB, DL, TII->get(X86::CMOV_GR32), cc)
>>               .addReg(cH32).addReg(cL32)
>>               .addImm(X86::COND_E);
>>
>>
Yes, and those are unaffected. This strictly only changes the cases where
the first element is an array subscript expression.

So, simplified there are three cases that relate to this:

1) MyObject.myFunction(AnArgument);
2) MyMap[MyKey].myFunction(AnArgument);
3) myFunction(AnArgument).myOtherFunction(AnArgument);

What I have observed is that quite commonly people split #1 after the "(",
presumably because an object and a function called on the object form a
stronger bond than the function and its argument. For #3, people seem to
more frequently see those to be builder-like and thus break before the "."
essentially ending up with two nicely balanced lines containing one
function call each. Now the fundamental question is, whether #2 is more
similar to #1 or more similar to #3.

I don't think it is worth a much more extensive discussion, simply because
this happens so rarely. There seems to be enough agreement about the
preference in LLVM/Clang and so I am going to change the implementation
accordingly.

Cheers,
Daniel


> in the various backends.
>>
>> -eric
>>
>>
>> On Sat, Jan 11, 2014 at 11:25 AM, dblaikie at gmail.com <dblaikie at gmail.com>
>> wrote:
>> > If we're voting, I'll put up a hand for the first style (breaking before
>> > '.') - but wouldn't mind seeing the real-world examples in LLVM that go
>> > either way to get a better sense of how this looks in the real world.
>> >
>> >
>> > On Fri Jan 10 2014 at 11:20:02 AM, Eric Christopher <echristo at gmail.com
>> >
>> > wrote:
>> >>
>> >>
>> >>
>> >> On Fri Jan 10 2014 at 1:43:37 AM, Daniel Jasper <djasper at google.com>
>> >> wrote:
>> >>
>> >> On Fri, Jan 10, 2014 at 10:07 AM, Chandler Carruth <
>> chandlerc at google.com>
>> >> wrote:
>> >>
>> >> On Fri, Jan 10, 2014 at 12:40 AM, Daniel Jasper <djasper at google.com>
>> >> wrote:
>> >>
>> >> Author: djasper
>> >> Date: Fri Jan 10 02:40:17 2014
>> >> New Revision: 198924
>> >>
>> >> URL: http://llvm.org/viewvc/llvm-project?rev=198924&view=rev
>> >> Log:
>> >> clang-format: Slightly adapt line break in edge case.
>> >>
>> >> Before:
>> >>   SomeMap[std::pair(aaaaaaaaaaaa, bbbbbbbbbbbbbbb)]
>> >>       .insert(ccccccccccccccccccccccc);
>> >>
>> >> After:
>> >>   SomeMap[std::pair(aaaaaaaaaaaa, bbbbbbbbbbbbbbb)].insert(
>> >>       ccccccccccccccccccccccc);
>> >>
>> >> This seems to be about 3:1 more common in Google and Chromium style
>> and I
>> >> found
>> >> only a handful of instances inside the LLVM codebase.
>> >>
>> >>
>> >> Without any significant numbers in the LLVM codebase, I would prefer
>> >> LLVM's style stick to the first -- I think its significantly easier to
>> read
>> >> personally. I'd also be interested in others' opinions since we don't
>> really
>> >> have numbers here.
>> >>
>> >>
>> >> Thanks for the additional input. There are less than 10 instances of
>> >> either choice in the LLVM codebase providing a bad signal-to-noise
>> ratio.
>> >> This might mean that it doesn't matter much, but I am happy to switch
>> this
>> >> back for LLVM style if there is a general preference.
>> >>
>> >>
>> >> Please. The second is much harder to read/scan and is a common pattern
>> >> (largely without long lines I guess) in the backend.
>> >>
>> >> -eric
>> >>
>> >>
>> >>
>> >>
>> >> Modified:
>> >>     cfe/trunk/lib/Format/TokenAnnotator.cpp
>> >>     cfe/trunk/unittests/Format/FormatTest.cpp
>> >>
>> >> Modified: cfe/trunk/lib/Format/TokenAnnotator.cpp
>> >> URL:
>> >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/
>> TokenAnnotator.cpp?rev=198924&r1=198923&r2=198924&view=diff
>> >>
>> >> ============================================================
>> ==================
>> >> --- cfe/trunk/lib/Format/TokenAnnotator.cpp (original)
>> >> +++ cfe/trunk/lib/Format/TokenAnnotator.cpp Fri Jan 10 02:40:17 2014
>> >> @@ -1203,7 +1203,7 @@ unsigned TokenAnnotator::splitPenalty(co
>> >>      return 2;
>> >>
>> >>    if (Right.isMemberAccess()) {
>> >> -    if (Left.isOneOf(tok::r_paren, tok::r_square) &&
>> Left.MatchingParen
>> >> &&
>> >> +    if (Left.is(tok::r_paren) && Left.MatchingParen &&
>> >>          Left.MatchingParen->ParameterCount > 0)
>> >>        return 20; // Should be smaller than breaking at a nested comma.
>> >>      return 150;
>> >>
>> >> Modified: cfe/trunk/unittests/Format/FormatTest.cpp
>> >> URL:
>> >> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/
>> Format/FormatTest.cpp?rev=198924&r1=198923&r2=198924&view=diff
>> >>
>> >> ============================================================
>> ==================
>> >> --- cfe/trunk/unittests/Format/FormatTest.cpp (original)
>> >> +++ cfe/trunk/unittests/Format/FormatTest.cpp Fri Jan 10 02:40:17 2014
>> >> @@ -3910,8 +3910,8 @@ TEST_F(FormatTest, WrapsAtFunctionCallsI
>> >>
>> >>    verifyFormat("EXPECT_CALL(SomeObject, SomeFunction(Parameter))\n"
>> >>                 "    .WillRepeatedly(Return(SomeValue));");
>> >> -  verifyFormat("SomeMap[std::pair(aaaaaaaaaaaa, bbbbbbbbbbbbbbb)]\n"
>> >> -               "    .insert(ccccccccccccccccccccccc);");
>> >> +  verifyFormat("SomeMap[std::pair(aaaaaaaaaaaa,
>> >> bbbbbbbbbbbbbbb)].insert(\n"
>> >> +               "    ccccccccccccccccccccccc);");
>> >>    verifyFormat("aaaaa(aaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,\n"
>> >>                 "
>> >> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa).aaaaa(aaaaa),\n"
>> >>                 "      aaaaaaaaaaaaaaaaaaaaa);");
>> >>
>> >>
>> >> _______________________________________________
>> >> cfe-commits mailing list
>> >> cfe-commits at cs.uiuc.edu
>> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> >>
>> >>
>> >
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140112/5d62dc2e/attachment.html>