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? > > >> >> >>> 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
