<div dir="ltr">Hi Jean-Philippe,<div><br></div><div>first off, sorry that this is causing you trouble.</div><div><br></div><div>I am always happy to reconsider. However, I do think that this is a good change based on hundreds of examples I have looked at. Obviously, this is subjective and I won't even start to make an argument on whether this is wasted or not.</div><div><br></div><div>We could add a separate option, but it is not quite as easy. AlwaysBreak has never been *always* break. There have always been exceptions. Two immediately spring to mind:</div><div>1. When the function name (plus open parenthesis) is shorter or equal to the ContinuationIndentWidth.</div><div>2. For nested blocks such as lambdas, at least in some of the cases.</div><div><br></div><div>Both are also about wasting space. I think single argument function calls are closely related to those two. So, we could add an "StrictAlwaysBreak" mode and then also not have the two exemptions above, but I am not sure it is worth it.</div><div><div class="gmail_extra"><br></div><div class="gmail_extra">How often do these cases occur in your codebase? Do you really feel like there is a significant readability disadvantage?</div><div class="gmail_extra"><br></div><div class="gmail_extra">Cheers,</div><div class="gmail_extra">Daniel</div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Mar 20, 2016 at 12:00 PM, Jean-philippe Dufraigne <span dir="ltr"><<a href="mailto:j.dufraigne@gmail.com" target="_blank" class="cremed">j.dufraigne@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Daniel,<div><br></div><div><div>I'm not sure I understand the logic behind modifying 'AlwaysBreak' to be 'AlwaysBreakExceptForSingleArgument'.</div><div><br></div><div>AlwaysBreak is always going to lead to "wasted space".</div><div>Some consider consistently break to not be a waste but actually contribute to readability, it seems that was what 'AlwaysBreak' was about.<br></div><div><br></div><div>Would you reconsider this change, or make it an option different from 'AlwaysBreak'?</div></div><div><br></div><div><br></div><div><br></div><div>We just completed the roll out clang-format at my company.</div><div><div>We are using: AlignAfterOpenBracket: AlwaysBreak</div></div><div>We tuned the settings to have a style that does not clash too much with our original style, and that will look like a regression for us.<br></div><div><br></div><div>Kind regards,</div><div>Jean-Philippe.</div><div><br></div></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">2016-03-17 12:00 GMT+00:00 Daniel Jasper via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank" class="cremed">cfe-commits@lists.llvm.org</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: djasper<br>
Date: Thu Mar 17 07:00:22 2016<br>
New Revision: 263709<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=263709&view=rev" rel="noreferrer" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project?rev=263709&view=rev</a><br>
Log:<br>
clang-format: Slightly weaken AlignAfterOpenBracket=AlwaysBreak.<br>
<br>
If a call takes a single argument, using AlwaysBreak can lead to lots<br>
of wasted lines and additional indentation without improving the<br>
readability in a significant way.<br>
<br>
Before:<br>
caaaaaaaaaaaall(<br>
caaaaaaaaaaaall(<br>
caaaaaaaaaaaall(<br>
caaaaaaaaaaaaaaaaaaaaaaall(aaaaaaaaaaaaaa, aaaaaaaaa))));<br>
<br>
After:<br>
caaaaaaaaaaaall(caaaaaaaaaaaall(caaaaaaaaaaaall(<br>
caaaaaaaaaaaaaaaaaaaaaaall(aaaaaaaaaaaaaa, aaaaaaaaa))));<br>
<br>
Modified:<br>
cfe/trunk/lib/Format/ContinuationIndenter.cpp<br>
cfe/trunk/unittests/Format/FormatTest.cpp<br>
<br>
Modified: cfe/trunk/lib/Format/ContinuationIndenter.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/ContinuationIndenter.cpp?rev=263709&r1=263708&r2=263709&view=diff" rel="noreferrer" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/ContinuationIndenter.cpp?rev=263709&r1=263708&r2=263709&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Format/ContinuationIndenter.cpp (original)<br>
+++ cfe/trunk/lib/Format/ContinuationIndenter.cpp Thu Mar 17 07:00:22 2016<br>
@@ -356,7 +356,17 @@ void ContinuationIndenter::addTokenOnCur<br>
Previous.isOneOf(tok::l_paren, TT_TemplateOpener, tok::l_square) &&<br>
State.Column > getNewLineColumn(State) &&<br>
(!Previous.Previous ||<br>
- !Previous.Previous->isOneOf(tok::kw_for, tok::kw_while, tok::kw_switch)))<br>
+ !Previous.Previous->isOneOf(tok::kw_for, tok::kw_while,<br>
+ tok::kw_switch)) &&<br>
+ // Don't do this for simple (no expressions) one-argument function calls<br>
+ // as that feels like needlessly wasting whitespace, e.g.:<br>
+ //<br>
+ // caaaaaaaaaaaall(<br>
+ // caaaaaaaaaaaall(<br>
+ // caaaaaaaaaaaall(<br>
+ // caaaaaaaaaaaaaaaaaaaaaaall(aaaaaaaaaaaaaa, aaaaaaaaa))));<br>
+ Current.FakeLParens.size() > 0 &&<br>
+ Current.FakeLParens.back() > prec::Unknown)<br>
State.Stack.back().NoLineBreak = true;<br>
<br>
if (Style.AlignAfterOpenBracket != FormatStyle::BAS_DontAlign &&<br>
<br>
Modified: cfe/trunk/unittests/Format/FormatTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=263709&r1=263708&r2=263709&view=diff" rel="noreferrer" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=263709&r1=263708&r2=263709&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/unittests/Format/FormatTest.cpp (original)<br>
+++ cfe/trunk/unittests/Format/FormatTest.cpp Thu Mar 17 07:00:22 2016<br>
@@ -4461,12 +4461,31 @@ TEST_F(FormatTest, AlignsAfterOpenBracke<br>
" aaaaaaaaaaa aaaaaaaaa,\n"<br>
" aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);",<br>
Style);<br>
- verifyFormat("SomeLongVariableName->someFunction(\n"<br>
- " foooooooo(\n"<br>
- " aaaaaaaaaaaaaaa,\n"<br>
- " aaaaaaaaaaaaaaaaaaaaa,\n"<br>
- " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa));",<br>
+ verifyFormat("SomeLongVariableName->someFunction(foooooooo(\n"<br>
+ " aaaaaaaaaaaaaaa,\n"<br>
+ " aaaaaaaaaaaaaaaaaaaaa,\n"<br>
+ " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa));",<br>
Style);<br>
+ verifyFormat(<br>
+ "aaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaa(\n"<br>
+ " aaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaa)));",<br>
+ Style);<br>
+ verifyFormat(<br>
+ "aaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaa.aaaaaaaaaa(\n"<br>
+ " aaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaa)));",<br>
+ Style);<br>
+ verifyFormat(<br>
+ "aaaaaaaaaaaaaaaaaaaaaaaa(\n"<br>
+ " aaaaaaaaaaaaaaaaaaaaa(\n"<br>
+ " aaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaa)),\n"<br>
+ " aaaaaaaaaaaaaaaa);",<br>
+ Style);<br>
+ verifyFormat(<br>
+ "aaaaaaaaaaaaaaaaaaaaaaaa(\n"<br>
+ " aaaaaaaaaaaaaaaaaaaaa(\n"<br>
+ " aaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaa)) &&\n"<br>
+ " aaaaaaaaaaaaaaaa);",<br>
+ Style);<br>
}<br>
<br>
TEST_F(FormatTest, ParenthesesAndOperandAlignment) {<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank" class="cremed">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank" class="cremed">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div></div></div>