On Wed, Nov 20, 2013 at 4:05 PM, Daniel Jasper <[email protected]> wrote:
> > > > On Wed, Nov 20, 2013 at 7:04 AM, Manuel Klimek <[email protected]> wrote: > >> On Wed, Nov 20, 2013 at 3:58 PM, Daniel Jasper <[email protected]>wrote: >> >>> >>> >>> >>> On Wed, Nov 20, 2013 at 3:20 AM, Manuel Klimek <[email protected]>wrote: >>> >>>> Author: klimek >>>> Date: Wed Nov 20 05:20:32 2013 >>>> New Revision: 195240 >>>> >>>> URL: http://llvm.org/viewvc/llvm-project?rev=195240&view=rev >>>> Log: >>>> Fix bug where optimization would lead to strange line breaks. >>>> >>>> Before: >>>> void f() { >>>> CHECK_EQ(aaaa, ( >>>> *bbbbbbbbb)->cccccc) >>>> << "qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq"; >>>> } >>>> >>>> After: >>>> void f() { >>>> CHECK_EQ(aaaa, (*bbbbbbbbb)->cccccc) >>>> << "qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq"; >>>> } >>>> >>>> Modified: >>>> cfe/trunk/lib/Format/ContinuationIndenter.cpp >>>> cfe/trunk/lib/Format/ContinuationIndenter.h >>>> cfe/trunk/unittests/Format/FormatTest.cpp >>>> >>>> Modified: cfe/trunk/lib/Format/ContinuationIndenter.cpp >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/ContinuationIndenter.cpp?rev=195240&r1=195239&r2=195240&view=diff >>>> >>>> ============================================================================== >>>> --- cfe/trunk/lib/Format/ContinuationIndenter.cpp (original) >>>> +++ cfe/trunk/lib/Format/ContinuationIndenter.cpp Wed Nov 20 05:20:32 >>>> 2013 >>>> @@ -224,13 +224,14 @@ unsigned ContinuationIndenter::addTokenT >>>> if (Newline) >>>> Penalty = addTokenOnNewLine(State, DryRun); >>>> else >>>> - addTokenOnCurrentLine(State, DryRun, ExtraSpaces); >>>> + Penalty = addTokenOnCurrentLine(State, DryRun, ExtraSpaces); >>>> >>>> return moveStateToNextToken(State, DryRun, Newline) + Penalty; >>>> } >>>> >>>> -void ContinuationIndenter::addTokenOnCurrentLine(LineState &State, >>>> bool DryRun, >>>> - unsigned ExtraSpaces) >>>> { >>>> +unsigned ContinuationIndenter::addTokenOnCurrentLine(LineState &State, >>>> + bool DryRun, >>>> + unsigned >>>> ExtraSpaces) { >>>> FormatToken &Current = *State.NextToken; >>>> const FormatToken &Previous = *State.NextToken->Previous; >>>> if (Current.is(tok::equal) && >>>> @@ -249,6 +250,15 @@ void ContinuationIndenter::addTokenOnCur >>>> State.Stack.back().LastSpace = State.Stack.back().VariablePos; >>>> } >>>> >>>> + unsigned Penalty = 0; >>>> + // A break before a "<<" will get Style.PenaltyBreakFirstLessLess, >>>> so a >>>> + // continuation with "<<" has a smaller penalty in general. >>>> + // If the LHS is long, we don't want to penalize the break though, >>>> so we >>>> + // also add Style.PenaltyBreakFirstLessLess. >>>> >>> >>> The comment is significantly harder to understand than the code itself >>> (I only understood it after I read the code). >>> >>> >>>> + if (Current.is(tok::lessless) && State.Stack.back().FirstLessLess == >>>> 0 && >>>> + State.Column > Style.ColumnLimit / 2) >>>> + Penalty += Style.PenaltyBreakFirstLessLess; >>>> >>> >>> 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. >>> >>> >>>> + >>>> unsigned Spaces = Current.SpacesRequiredBefore + ExtraSpaces; >>>> >>>> if (!DryRun) >>>> @@ -307,6 +317,7 @@ void ContinuationIndenter::addTokenOnCur >>>> State.Stack[State.Stack.size() - 2].CallContinuation == 0) >>>> State.Stack.back().LastSpace = State.Column; >>>> } >>>> + return Penalty; >>>> } >>>> >>>> unsigned ContinuationIndenter::addTokenOnNewLine(LineState &State, >>>> @@ -332,9 +343,8 @@ unsigned ContinuationIndenter::addTokenO >>>> Penalty += State.NextToken->SplitPenalty; >>>> >>>> // Breaking before the first "<<" is generally not desirable if the >>>> LHS is >>>> - // short. >>>> - if (Current.is(tok::lessless) && State.Stack.back().FirstLessLess == >>>> 0 && >>>> - State.Column <= Style.ColumnLimit / 2) >>>> + // short (not breaking with a long LHS is penalized in >>>> addTokenOnCurrentLine). >>>> + if (Current.is(tok::lessless) && State.Stack.back().FirstLessLess == >>>> 0) >>>> >>> >>> Simply adding the penalty whenever the LHS broken should do almost the >>> same thing. Did that instead in r195253. >>> >> >> 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. >> > > How is anything relying on an indent? > Are you asking this question because you make the distinction between indent and alignment? > > >> >> >>> >>> >>>> Penalty += Style.PenaltyBreakFirstLessLess; >>>> >>>> if (Current.is(tok::l_brace) && Current.BlockKind == BK_Block) { >>>> >>>> Modified: cfe/trunk/lib/Format/ContinuationIndenter.h >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/ContinuationIndenter.h?rev=195240&r1=195239&r2=195240&view=diff >>>> >>>> ============================================================================== >>>> --- cfe/trunk/lib/Format/ContinuationIndenter.h (original) >>>> +++ cfe/trunk/lib/Format/ContinuationIndenter.h Wed Nov 20 05:20:32 2013 >>>> @@ -91,8 +91,8 @@ private: >>>> /// >>>> /// If \p DryRun is \c false, also creates and stores the required >>>> /// \c Replacement. >>>> - void addTokenOnCurrentLine(LineState &State, bool DryRun, >>>> - unsigned ExtraSpaces); >>>> + unsigned addTokenOnCurrentLine(LineState &State, bool DryRun, >>>> + unsigned ExtraSpaces); >>>> >>>> /// \brief Appends the next token to \p State and updates information >>>> /// necessary for indentation. >>>> >>>> Modified: cfe/trunk/unittests/Format/FormatTest.cpp >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=195240&r1=195239&r2=195240&view=diff >>>> >>>> ============================================================================== >>>> --- cfe/trunk/unittests/Format/FormatTest.cpp (original) >>>> +++ cfe/trunk/unittests/Format/FormatTest.cpp Wed Nov 20 05:20:32 2013 >>>> @@ -3744,6 +3744,11 @@ TEST_F(FormatTest, AlignsPipes) { >>>> EXPECT_EQ("llvm::errs() << \"\n" >>>> " << a;", >>>> format("llvm::errs() << \"\n<<a;")); >>>> + >>>> + verifyFormat("void f() {\n" >>>> + " CHECK_EQ(aaaa, (*bbbbbbbbb)->cccccc)\n" >>>> + " << \"qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq\";\n" >>>> + "}"); >>>> } >>>> >>>> TEST_F(FormatTest, UnderstandsEquals) { >>>> >>>> >>>> _______________________________________________ >>>> 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
