r195240 - Fix bug where optimization would lead to strange line breaks.

[Daniel Jasper]

On Wed, Nov 20, 2013 at 7:04 AM, Manuel Klimek <klimek at google.com> wrote:

> On Wed, Nov 20, 2013 at 3:58 PM, Daniel Jasper <djasper at google.com> wrote:
>
>>
>>
>>
>> On Wed, Nov 20, 2013 at 3:20 AM, Manuel Klimek <klimek at google.com> 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
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131120/ba17c418/attachment.html>