r195240 - Fix bug where optimization would lead to strange line breaks.
Daniel Jasper
djasper at google.com
Wed Nov 20 07:16:28 PST 2013
On Wed, Nov 20, 2013 at 7:12 AM, Manuel Klimek <klimek at google.com> wrote:
> On Wed, Nov 20, 2013 at 4:05 PM, Daniel Jasper <djasper at google.com> wrote:
>
>>
>>
>>
>> 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?
>>
>
> 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
>>>>> 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/2b17e09d/attachment.html>
More information about the cfe-commits
mailing list