[cfe-commits] r173273 - in /cfe/trunk: lib/Format/Format.cpp unittests/Format/FormatTest.cpp

[Daniel Jasper]

Looking at the added / changed test cases that currently does not help much
/ would be harmful. I think what we'll need to do is change the formatter
to actually try multiple indents on the same line break and then assign
different penalties to them. This might help with many of these things.
However, we don't do this yet.


On Wed, Jan 23, 2013 at 11:10 PM, Nico Weber <thakis at chromium.org> wrote:

> On Wed, Jan 23, 2013 at 8:58 AM, Daniel Jasper <djasper at google.com> wrote:
>
>> Author: djasper
>> Date: Wed Jan 23 10:58:21 2013
>> New Revision: 173273
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=173273&view=rev
>> Log:
>> Don't try to align builder-type continuations on assignments.
>>
>> Before:
>> int aaaa = aaaaa().aaaaa() // force break
>>            .aaaaa();
>> After:
>> int aaaa = aaaaa().aaaaa() // force break
>>     .aaaaa();
>>
>
> Since this pattern is explicitly detected already, should this try to
> align on the '.'?
>
>
>>
>> The other indent is just wrong and confusing.
>>
>> Modified:
>>     cfe/trunk/lib/Format/Format.cpp
>>     cfe/trunk/unittests/Format/FormatTest.cpp
>>
>> Modified: cfe/trunk/lib/Format/Format.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=173273&r1=173272&r2=173273&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/Format/Format.cpp (original)
>> +++ cfe/trunk/lib/Format/Format.cpp Wed Jan 23 10:58:21 2013
>> @@ -421,9 +421,9 @@
>>
>>    struct ParenState {
>>      ParenState(unsigned Indent, unsigned LastSpace)
>> -        : Indent(Indent), LastSpace(LastSpace), FirstLessLess(0),
>> -          BreakBeforeClosingBrace(false), BreakAfterComma(false),
>> -          HasMultiParameterLine(false) {}
>> +        : Indent(Indent), LastSpace(LastSpace), AssignmentColumn(0),
>> +          FirstLessLess(0), BreakBeforeClosingBrace(false),
>> +          BreakAfterComma(false), HasMultiParameterLine(false) {}
>>
>>      /// \brief The position to which a specific parenthesis level needs
>> to be
>>      /// indented.
>> @@ -436,6 +436,9 @@
>>      ///                             OtherParameter));
>>      unsigned LastSpace;
>>
>> +    /// \brief This is the column of the first token after an assignment.
>> +    unsigned AssignmentColumn;
>> +
>>      /// \brief The position the first "<<" operator encountered on each
>> level.
>>      ///
>>      /// Used to align "<<" operators. 0 if no such operator has been
>> encountered
>> @@ -457,6 +460,8 @@
>>          return Indent < Other.Indent;
>>        if (LastSpace != Other.LastSpace)
>>          return LastSpace < Other.LastSpace;
>> +      if (AssignmentColumn != Other.AssignmentColumn)
>> +        return AssignmentColumn < Other.AssignmentColumn;
>>        if (FirstLessLess != Other.FirstLessLess)
>>          return FirstLessLess < Other.FirstLessLess;
>>        if (BreakBeforeClosingBrace != Other.BreakBeforeClosingBrace)
>> @@ -547,6 +552,9 @@
>>          State.Column = State.ForLoopVariablePos;
>>        } else if (State.NextToken->Parent->ClosesTemplateDeclaration) {
>>          State.Column = State.Stack[ParenLevel].Indent - 4;
>> +      } else if (Previous.Type == TT_BinaryOperator &&
>> +                 State.Stack.back().AssignmentColumn != 0) {
>> +        State.Column = State.Stack.back().AssignmentColumn;
>>        } else {
>>          State.Column = State.Stack[ParenLevel].Indent;
>>        }
>> @@ -587,7 +595,7 @@
>>        if (RootToken.isNot(tok::kw_for) && ParenLevel == 0 &&
>>            (getPrecedence(Previous) == prec::Assignment ||
>>             Previous.is(tok::kw_return)))
>> -        State.Stack[ParenLevel].Indent = State.Column + Spaces;
>> +        State.Stack.back().AssignmentColumn = State.Column + Spaces;
>>        if (Previous.is(tok::l_paren) || Previous.is(tok::l_brace) ||
>>            State.NextToken->Parent->Type == TT_TemplateOpener)
>>          State.Stack[ParenLevel].Indent = State.Column + Spaces;
>>
>> Modified: cfe/trunk/unittests/Format/FormatTest.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=173273&r1=173272&r2=173273&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/unittests/Format/FormatTest.cpp (original)
>> +++ cfe/trunk/unittests/Format/FormatTest.cpp Wed Jan 23 10:58:21 2013
>> @@ -1006,10 +1006,10 @@
>>  TEST_F(FormatTest, FormatsBuilderPattern) {
>>    verifyFormat(
>>        "return llvm::StringSwitch<Reference::Kind>(name)\n"
>> -      "       .StartsWith(\".eh_frame_hdr\", ORDER_EH_FRAMEHDR)\n"
>> -      "       .StartsWith(\".eh_frame\",
>> ORDER_EH_FRAME).StartsWith(\".init\", ORDER_INIT)\n"
>> -      "       .StartsWith(\".fini\", ORDER_FINI).StartsWith(\".hash\",
>> ORDER_HASH)\n"
>> -      "       .Default(ORDER_TEXT);\n");
>> +      "    .StartsWith(\".eh_frame_hdr\", ORDER_EH_FRAMEHDR)\n"
>> +      "    .StartsWith(\".eh_frame\",
>> ORDER_EH_FRAME).StartsWith(\".init\", ORDER_INIT)\n"
>> +      "    .StartsWith(\".fini\", ORDER_FINI).StartsWith(\".hash\",
>> ORDER_HASH)\n"
>> +      "    .Default(ORDER_TEXT);\n");
>>  }
>>
>>  TEST_F(FormatTest, DoesNotBreakTrailingAnnotation) {
>> @@ -1042,6 +1042,10 @@
>>    verifyFormat(
>>        "CharSourceRange LineRange = CharSourceRange::getTokenRange(\n"
>>        "    Line.Tokens.front().Tok.getLo(),
>> Line.Tokens.back().Tok.getLoc());");
>> +
>> +  verifyFormat(
>> +      "aaaaaaaaaaaaaaaaaaaaaaaaaa aaaa = aaaaaaaaaaaaaa(0).aaaa()\n"
>> +      "    .aaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaa::aaaaaaaaaaaaaaaaaaaaa);");
>>  }
>>
>>  TEST_F(FormatTest, AlignsAfterAssignments) {
>>
>>
>> _______________________________________________
>> 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/20130124/3ed6053b/attachment.html>