On Wed, Nov 20, 2013 at 7:12 AM, Manuel Klimek <[email protected]> wrote:
> 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? > 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. > > > >> >> >>> >>> >>>> >>>> >>>>> 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
