<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Nov 20, 2013 at 7:39 AM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank" class="cremed">klimek@google.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"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5">On Wed, Nov 20, 2013 at 4:29 PM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank" class="cremed">djasper@google.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"><br><div class="gmail_extra"><br><br><div class="gmail_quote"><div><div>On Wed, Nov 20, 2013 at 7:19 AM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank" class="cremed">klimek@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><div>On Wed, Nov 20, 2013 at 4:16 PM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank" class="cremed">djasper@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">
<div><div>On Wed, Nov 20, 2013 at 7:12 AM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank" class="cremed">klimek@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><div>On Wed, Nov 20, 2013 at 4:05 PM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank" class="cremed">djasper@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">
<div><div>On Wed, Nov 20, 2013 at 7:04 AM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank" class="cremed">klimek@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><div>On Wed, Nov 20, 2013 at 3:58 PM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank" class="cremed">djasper@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">
<div><div>On Wed, Nov 20, 2013 at 3:20 AM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank" class="cremed">klimek@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Author: klimek<br>
Date: Wed Nov 20 05:20:32 2013<br>
New Revision: 195240<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=195240&view=rev" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project?rev=195240&view=rev</a><br>
Log:<br>
Fix bug where optimization would lead to strange line breaks.<br>
<br>
Before:<br>
void f() {<br>
CHECK_EQ(aaaa, (<br>
*bbbbbbbbb)->cccccc)<br>
<< "qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq";<br>
}<br>
<br>
After:<br>
void f() {<br>
CHECK_EQ(aaaa, (*bbbbbbbbb)->cccccc)<br>
<< "qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq";<br>
}<br>
<br>
Modified:<br>
cfe/trunk/lib/Format/ContinuationIndenter.cpp<br>
cfe/trunk/lib/Format/ContinuationIndenter.h<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=195240&r1=195239&r2=195240&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/ContinuationIndenter.cpp?rev=195240&r1=195239&r2=195240&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Format/ContinuationIndenter.cpp (original)<br>
+++ cfe/trunk/lib/Format/ContinuationIndenter.cpp Wed Nov 20 05:20:32 2013<br>
@@ -224,13 +224,14 @@ unsigned ContinuationIndenter::addTokenT<br>
if (Newline)<br>
Penalty = addTokenOnNewLine(State, DryRun);<br>
else<br>
- addTokenOnCurrentLine(State, DryRun, ExtraSpaces);<br>
+ Penalty = addTokenOnCurrentLine(State, DryRun, ExtraSpaces);<br>
<br>
return moveStateToNextToken(State, DryRun, Newline) + Penalty;<br>
}<br>
<br>
-void ContinuationIndenter::addTokenOnCurrentLine(LineState &State, bool DryRun,<br>
- unsigned ExtraSpaces) {<br>
+unsigned ContinuationIndenter::addTokenOnCurrentLine(LineState &State,<br>
+ bool DryRun,<br>
+ unsigned ExtraSpaces) {<br>
FormatToken &Current = *State.NextToken;<br>
const FormatToken &Previous = *State.NextToken->Previous;<br>
if (Current.is(tok::equal) &&<br>
@@ -249,6 +250,15 @@ void ContinuationIndenter::addTokenOnCur<br>
State.Stack.back().LastSpace = State.Stack.back().VariablePos;<br>
}<br>
<br>
+ unsigned Penalty = 0;<br>
+ // A break before a "<<" will get Style.PenaltyBreakFirstLessLess, so a<br>
+ // continuation with "<<" has a smaller penalty in general.<br>
+ // If the LHS is long, we don't want to penalize the break though, so we<br>
+ // also add Style.PenaltyBreakFirstLessLess.<br></blockquote><div><br></div></div></div><div>The comment is significantly harder to understand than the code itself (I only understood it after I read the code).</div><div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ if (Current.is(tok::lessless) && State.Stack.back().FirstLessLess == 0 &&<br>
+ State.Column > Style.ColumnLimit / 2)<br>
+ Penalty += Style.PenaltyBreakFirstLessLess;<br></blockquote><div><br></div></div><div>I don't like this approach as the logic behind it is very convoluted and now spread over multiple code sites (in addition to addTokenOnNewLine and addTokenOnCurrentLine it also relies on mustBreak for correct behavior). Also adding a PenaltyBREAK... when not actually breaking seems bad to me.</div>
<div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+<br>
unsigned Spaces = Current.SpacesRequiredBefore + ExtraSpaces;<br>
<br>
if (!DryRun)<br>
@@ -307,6 +317,7 @@ void ContinuationIndenter::addTokenOnCur<br>
State.Stack[State.Stack.size() - 2].CallContinuation == 0)<br>
State.Stack.back().LastSpace = State.Column;<br>
}<br>
+ return Penalty;<br>
}<br>
<br>
unsigned ContinuationIndenter::addTokenOnNewLine(LineState &State,<br>
@@ -332,9 +343,8 @@ unsigned ContinuationIndenter::addTokenO<br>
Penalty += State.NextToken->SplitPenalty;<br>
<br>
// Breaking before the first "<<" is generally not desirable if the LHS is<br>
- // short.<br>
- if (Current.is(tok::lessless) && State.Stack.back().FirstLessLess == 0 &&<br>
- State.Column <= Style.ColumnLimit / 2)<br>
+ // short (not breaking with a long LHS is penalized in addTokenOnCurrentLine).<br>
+ if (Current.is(tok::lessless) && State.Stack.back().FirstLessLess == 0)<br></blockquote><div><br></div></div><div>Simply adding the penalty whenever the LHS broken should do almost the same thing. Did that instead in r195253.</div>
<div><div>
<div></div></div></div></div></div></div></blockquote><div><br></div></div></div><div>I think that solution is worse, as it still doesn't get rid of the very subtle coupling between the previous line's indent and the penalty where the previous line shouldn't matter (in this case we're breaking, after all). It definitely was very confusing to me - it took me forever to understand what the current code was doing, and what it was actually trying to do.</div>
</div></div></div></blockquote><div><br></div></div></div><div>How is anything relying on an indent?</div></div></div></div></blockquote><div><br></div></div></div><div>Are you asking this question because you make the distinction between indent and alignment?</div>
</div></div></div></blockquote><div><br></div></div></div><div>No. I am probably completely not understanding you. All this does is look at the current column. If that is past half of the column limit, we add a penalty. Indentation/alignment is not involved.</div>
</div></div></div></blockquote><div><br></div></div></div><div>Well, we're breaking, so it's the "previous end of line column".</div></div></div></div></blockquote><div><br></div></div></div><div>That has nothing to do with alignment or indentation in this case.</div>
<div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>To rephrase, I think the problem really is that we want to give a negative penalty to "not breaking a first lessless that's in the left half of the screen".</div></div></div></div></blockquote><div><br>
</div></div><div>No, that is not what we want to do. Especially, we do not want to give this negative penalty when the LHS is split over multiple lines. So I'd phrase it as a negative penalty if we can keep the "<<" on the first line and in the left part of the screen (although the latter is more of an exception). </div>
<div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>My proposed solution does this by giving penalties to the inverse (break before lessless and continuation of first lessless in the second half of the screen).</div><div><br></div><div>From reading the current implementation, I don't understand that that's what's going on, or why looking whether the line was broken before has anything to do with it. I think that's very subtle.</div>
</div></div></div></blockquote><div><br></div></div><div>I think your solution is way more subtle as it depends on a number of other things. For instance it took me some time to convince myself that it would not create something like:</div>
<div><br></div><div>Diag(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,</div><div> aaa) << "test";</div><div><br></div><div>Instead of:</div><div><div><br></div><div>Diag(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, aaa)</div>
<div> << "test";</div></div><div><br></div><div>Which it doesn't because it relies on behavior in mustBreak().</div></div></div></div></blockquote><div><br></div></div></div><div>I actually would rather make that explicit in the addTokenToCurrentLine.</div>
<div><br></div><div>My main problem with the penalty-on-break depending on the previous line's length is that I completely didn't expect a line break where the alignment doesn't depend on the previous line's length to have a penalty that depends on the previous line's length.</div>
</div></div></div></blockquote><div><br></div><div>It is not the previous line length, though. It is the current line length right before inserting something on the next line :-).</div><div><br></div><div>To me it is much more confusing to add a penalty in one code path and then negating that in another code path by adding the same penalty to all other solutions. I would give it a high chance that some day we alter one of the code paths introducing (probably latent) bugs.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>For example, if we had other state options than breaks that changed the length of the line, it would lead to the same problem we had now.</div>
</div></div></div></blockquote><div><br></div><div>The same is true for your solution just in the opposite way (yours would try to make the line unnecessarily short).</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Anyway, both ways are subtle and we wasted enough time implementing and reimplementing it, so I'll stop arguing now...</div></div></div></div></blockquote>
<div><br></div><div>I agree 100%.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><div>
<div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><div><div><div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">
<div class="gmail_extra"><div class="gmail_quote"><div><div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Penalty += Style.PenaltyBreakFirstLessLess;<br>
<br>
if (Current.is(tok::l_brace) && Current.BlockKind == BK_Block) {<br>
<br>
Modified: cfe/trunk/lib/Format/ContinuationIndenter.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/ContinuationIndenter.h?rev=195240&r1=195239&r2=195240&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/ContinuationIndenter.h?rev=195240&r1=195239&r2=195240&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Format/ContinuationIndenter.h (original)<br>
+++ cfe/trunk/lib/Format/ContinuationIndenter.h Wed Nov 20 05:20:32 2013<br>
@@ -91,8 +91,8 @@ private:<br>
///<br>
/// If \p DryRun is \c false, also creates and stores the required<br>
/// \c Replacement.<br>
- void addTokenOnCurrentLine(LineState &State, bool DryRun,<br>
- unsigned ExtraSpaces);<br>
+ unsigned addTokenOnCurrentLine(LineState &State, bool DryRun,<br>
+ unsigned ExtraSpaces);<br>
<br>
/// \brief Appends the next token to \p State and updates information<br>
/// necessary for indentation.<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=195240&r1=195239&r2=195240&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=195240&r1=195239&r2=195240&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/unittests/Format/FormatTest.cpp (original)<br>
+++ cfe/trunk/unittests/Format/FormatTest.cpp Wed Nov 20 05:20:32 2013<br>
@@ -3744,6 +3744,11 @@ TEST_F(FormatTest, AlignsPipes) {<br>
EXPECT_EQ("llvm::errs() << \"\n"<br>
" << a;",<br>
format("llvm::errs() << \"\n<<a;"));<br>
+<br>
+ verifyFormat("void f() {\n"<br>
+ " CHECK_EQ(aaaa, (*bbbbbbbbb)->cccccc)\n"<br>
+ " << \"qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq\";\n"<br>
+ "}");<br>
}<br>
<br>
TEST_F(FormatTest, UnderstandsEquals) {<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank" class="cremed">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div><br></div></div>
</blockquote></div><br></div></div>