<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">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>The comment is significantly harder to understand than the code itself (I only understood it after I read the code).</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>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><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>Simply adding the penalty whenever the LHS broken should do almost the same thing. Did that instead in r195253.</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" 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><br></div></div>