On Sat, Jan 11, 2014 at 8:43 PM, [email protected] <[email protected]>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 <[email protected]> > 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, [email protected] <[email protected]> >> 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 <[email protected] >> > >> > wrote: >> >> >> >> >> >> >> >> On Fri Jan 10 2014 at 1:43:37 AM, Daniel Jasper <[email protected]> >> >> wrote: >> >> >> >> On Fri, Jan 10, 2014 at 10:07 AM, Chandler Carruth < >> [email protected]> >> >> wrote: >> >> >> >> On Fri, Jan 10, 2014 at 12:40 AM, Daniel Jasper <[email protected]> >> >> 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 >> >> [email protected] >> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> >> >> >> >> > >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
