For reference, this was reviewed in https://reviews.llvm.org/D21279
(Please always include review links in the future.) On Wed, Mar 22, 2017 at 7:51 PM, Nikola Smiljanic via cfe-commits <cfe-commits@lists.llvm.org> wrote: > Author: nikola > Date: Wed Mar 22 21:51:25 2017 > New Revision: 298574 > > URL: http://llvm.org/viewvc/llvm-project?rev=298574&view=rev > Log: > Fix issues in clang-format's AlignConsecutive modes. > > Patch by Ben Harper. > > Modified: > cfe/trunk/lib/Format/WhitespaceManager.cpp > cfe/trunk/lib/Format/WhitespaceManager.h > cfe/trunk/unittests/Format/FormatTest.cpp > > Modified: cfe/trunk/lib/Format/WhitespaceManager.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/WhitespaceManager.cpp?rev=298574&r1=298573&r2=298574&view=diff > ============================================================================== > --- cfe/trunk/lib/Format/WhitespaceManager.cpp (original) > +++ cfe/trunk/lib/Format/WhitespaceManager.cpp Wed Mar 22 21:51:25 2017 > @@ -50,9 +50,9 @@ void WhitespaceManager::replaceWhitespac > if (Tok.Finalized) > return; > Tok.Decision = (Newlines > 0) ? FD_Break : FD_Continue; > - Changes.push_back(Change(Tok, /*CreateReplacement=*/true, > - Tok.WhitespaceRange, Spaces, StartOfTokenColumn, > - Newlines, "", "", InPPDirective && !Tok.IsFirst, > + Changes.push_back(Change(Tok, /*CreateReplacement=*/true, > Tok.WhitespaceRange, > + Spaces, StartOfTokenColumn, Newlines, "", "", > + InPPDirective && !Tok.IsFirst, > /*IsInsideToken=*/false)); > } > > @@ -192,21 +192,56 @@ AlignTokenSequence(unsigned Start, unsig > SmallVector<WhitespaceManager::Change, 16> &Changes) { > bool FoundMatchOnLine = false; > int Shift = 0; > + > + // ScopeStack keeps track of the current scope depth. It contains indices > of > + // the first token on each scope. > + // We only run the "Matches" function on tokens from the outer-most scope. > + // However, we do need to pay special attention to one class of tokens > + // that are not in the outer-most scope, and that is function parameters > + // which are split across multiple lines, as illustrated by this example: > + // double a(int x); > + // int b(int y, > + // double z); > + // In the above example, we need to take special care to ensure that > + // 'double z' is indented along with it's owning function 'b'. > + SmallVector<unsigned, 16> ScopeStack; > + > for (unsigned i = Start; i != End; ++i) { > - if (Changes[i].NewlinesBefore > 0) { > - FoundMatchOnLine = false; > + if (ScopeStack.size() != 0 && > + Changes[i].nestingAndIndentLevel() < > + Changes[ScopeStack.back()].nestingAndIndentLevel()) > + ScopeStack.pop_back(); > + > + if (i != Start && Changes[i].nestingAndIndentLevel() > > + Changes[i - 1].nestingAndIndentLevel()) > + ScopeStack.push_back(i); > + > + bool InsideNestedScope = ScopeStack.size() != 0; > + > + if (Changes[i].NewlinesBefore > 0 && !InsideNestedScope) { > Shift = 0; > + FoundMatchOnLine = false; > } > > // If this is the first matching token to be aligned, remember by how > many > // spaces it has to be shifted, so the rest of the changes on the line > are > // shifted by the same amount > - if (!FoundMatchOnLine && Matches(Changes[i])) { > + if (!FoundMatchOnLine && !InsideNestedScope && Matches(Changes[i])) { > FoundMatchOnLine = true; > Shift = Column - Changes[i].StartOfTokenColumn; > Changes[i].Spaces += Shift; > } > > + // This is for function parameters that are split across multiple lines, > + // as mentioned in the ScopeStack comment. > + if (InsideNestedScope && Changes[i].NewlinesBefore > 0) { > + unsigned ScopeStart = ScopeStack.back(); > + if (Changes[ScopeStart - 1].Tok->is(TT_FunctionDeclarationName) || > + (ScopeStart > Start + 1 && > + Changes[ScopeStart - 2].Tok->is(TT_FunctionDeclarationName))) > + Changes[i].Spaces += Shift; > + } > + > assert(Shift >= 0); > Changes[i].StartOfTokenColumn += Shift; > if (i + 1 != Changes.size()) > @@ -214,15 +249,37 @@ AlignTokenSequence(unsigned Start, unsig > } > } > > -// Walk through all of the changes and find sequences of matching tokens to > -// align. To do so, keep track of the lines and whether or not a matching > token > -// was found on a line. If a matching token is found, extend the current > -// sequence. If the current line cannot be part of a sequence, e.g. because > -// there is an empty line before it or it contains only non-matching tokens, > -// finalize the previous sequence. > +// Walk through a subset of the changes, starting at StartAt, and find > +// sequences of matching tokens to align. To do so, keep track of the lines > and > +// whether or not a matching token was found on a line. If a matching token > is > +// found, extend the current sequence. If the current line cannot be part of > a > +// sequence, e.g. because there is an empty line before it or it contains > only > +// non-matching tokens, finalize the previous sequence. > +// The value returned is the token on which we stopped, either because we > +// exhausted all items inside Changes, or because we hit a scope level higher > +// than our initial scope. > +// This function is recursive. Each invocation processes only the scope level > +// equal to the initial level, which is the level of Changes[StartAt]. > +// If we encounter a scope level greater than the initial level, then we call > +// ourselves recursively, thereby avoiding the pollution of the current state > +// with the alignment requirements of the nested sub-level. This recursive > +// behavior is necessary for aligning function prototypes that have one or > more > +// arguments. > +// If this function encounters a scope level less than the initial level, > +// it returns the current position. > +// There is a non-obvious subtlety in the recursive behavior: Even though we > +// defer processing of nested levels to recursive invocations of this > +// function, when it comes time to align a sequence of tokens, we run the > +// alignment on the entire sequence, including the nested levels. > +// When doing so, most of the nested tokens are skipped, because their > +// alignment was already handled by the recursive invocations of this > function. > +// However, the special exception is that we do NOT skip function parameters > +// that are split across multiple lines. See the test case in FormatTest.cpp > +// that mentions "split function parameter alignment" for an example of this. > template <typename F> > -static void AlignTokens(const FormatStyle &Style, F &&Matches, > - SmallVector<WhitespaceManager::Change, 16> &Changes) > { > +static unsigned AlignTokens(const FormatStyle &Style, F &&Matches, > + SmallVector<WhitespaceManager::Change, 16> > &Changes, > + unsigned StartAt) { > unsigned MinColumn = 0; > unsigned MaxColumn = UINT_MAX; > > @@ -230,14 +287,11 @@ static void AlignTokens(const FormatStyl > unsigned StartOfSequence = 0; > unsigned EndOfSequence = 0; > > - // Keep track of the nesting level of matching tokens, i.e. the number of > - // surrounding (), [], or {}. We will only align a sequence of matching > - // token that share the same scope depth. > - // > - // FIXME: This could use FormatToken::NestingLevel information, but there > is > - // an outstanding issue wrt the brace scopes. > - unsigned NestingLevelOfLastMatch = 0; > - unsigned NestingLevel = 0; > + // Measure the scope level (i.e. depth of (), [], {}) of the first token, > and > + // abort when we hit any token in a higher scope than the starting one. > + auto NestingAndIndentLevel = StartAt < Changes.size() > + ? Changes[StartAt].nestingAndIndentLevel() > + : std::pair<unsigned, unsigned>(0, 0); > > // Keep track of the number of commas before the matching tokens, we will > only > // align a sequence of matching tokens if they are preceded by the same > number > @@ -265,7 +319,11 @@ static void AlignTokens(const FormatStyl > EndOfSequence = 0; > }; > > - for (unsigned i = 0, e = Changes.size(); i != e; ++i) { > + unsigned i = StartAt; > + for (unsigned e = Changes.size(); i != e; ++i) { > + if (Changes[i].nestingAndIndentLevel() < NestingAndIndentLevel) > + break; > + > if (Changes[i].NewlinesBefore != 0) { > CommasBeforeMatch = 0; > EndOfSequence = i; > @@ -279,29 +337,22 @@ static void AlignTokens(const FormatStyl > > if (Changes[i].Tok->is(tok::comma)) { > ++CommasBeforeMatch; > - } else if (Changes[i].Tok->isOneOf(tok::r_brace, tok::r_paren, > - tok::r_square)) { > - --NestingLevel; > - } else if (Changes[i].Tok->isOneOf(tok::l_brace, tok::l_paren, > - tok::l_square)) { > - // We want sequences to skip over child scopes if possible, but not the > - // other way around. > - NestingLevelOfLastMatch = std::min(NestingLevelOfLastMatch, > NestingLevel); > - ++NestingLevel; > + } else if (Changes[i].nestingAndIndentLevel() > NestingAndIndentLevel) { > + // Call AlignTokens recursively, skipping over this scope block. > + unsigned StoppedAt = AlignTokens(Style, Matches, Changes, i); > + i = StoppedAt - 1; > + continue; > } > > if (!Matches(Changes[i])) > continue; > > // If there is more than one matching token per line, or if the number of > - // preceding commas, or the scope depth, do not match anymore, end the > - // sequence. > - if (FoundMatchOnLine || CommasBeforeMatch != CommasBeforeLastMatch || > - NestingLevel != NestingLevelOfLastMatch) > + // preceding commas, do not match anymore, end the sequence. > + if (FoundMatchOnLine || CommasBeforeMatch != CommasBeforeLastMatch) > AlignCurrentSequence(); > > CommasBeforeLastMatch = CommasBeforeMatch; > - NestingLevelOfLastMatch = NestingLevel; > FoundMatchOnLine = true; > > if (StartOfSequence == 0) > @@ -324,8 +375,9 @@ static void AlignTokens(const FormatStyl > MaxColumn = std::min(MaxColumn, ChangeMaxColumn); > } > > - EndOfSequence = Changes.size(); > + EndOfSequence = i; > AlignCurrentSequence(); > + return i; > } > > void WhitespaceManager::alignConsecutiveAssignments() { > @@ -344,7 +396,7 @@ void WhitespaceManager::alignConsecutive > > return C.Tok->is(tok::equal); > }, > - Changes); > + Changes, /*StartAt=*/0); > } > > void WhitespaceManager::alignConsecutiveDeclarations() { > @@ -357,13 +409,15 @@ void WhitespaceManager::alignConsecutive > // const char* const* v1; > // float const* v2; > // SomeVeryLongType const& v3; > - > AlignTokens(Style, > [](Change const &C) { > - return C.Tok->isOneOf(TT_StartOfName, > - TT_FunctionDeclarationName); > + // tok::kw_operator is necessary for aligning operator > overload > + // definitions. > + return C.Tok->is(TT_StartOfName) || > + C.Tok->is(TT_FunctionDeclarationName) || > + C.Tok->is(tok::kw_operator); > }, > - Changes); > + Changes, /*StartAt=*/0); > } > > void WhitespaceManager::alignTrailingComments() { > > Modified: cfe/trunk/lib/Format/WhitespaceManager.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/WhitespaceManager.h?rev=298574&r1=298573&r2=298574&view=diff > ============================================================================== > --- cfe/trunk/lib/Format/WhitespaceManager.h (original) > +++ cfe/trunk/lib/Format/WhitespaceManager.h Wed Mar 22 21:51:25 2017 > @@ -149,6 +149,14 @@ public: > // the alignment process. > const Change *StartOfBlockComment; > int IndentationOffset; > + > + // A combination of nesting level and indent level, which are used in > + // tandem to compute lexical scope, for the purposes of deciding > + // when to stop consecutive alignment runs. > + std::pair<unsigned, unsigned> > + nestingAndIndentLevel() const { > + return std::make_pair(Tok->NestingLevel, Tok->IndentLevel); > + } > }; > > private: > > Modified: cfe/trunk/unittests/Format/FormatTest.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=298574&r1=298573&r2=298574&view=diff > ============================================================================== > --- cfe/trunk/unittests/Format/FormatTest.cpp (original) > +++ cfe/trunk/unittests/Format/FormatTest.cpp Wed Mar 22 21:51:25 2017 > @@ -7672,12 +7672,11 @@ TEST_F(FormatTest, AlignConsecutiveAssig > "};", > Alignment); > > - // FIXME: Should align all three assignments > verifyFormat( > "int i = 1;\n" > "SomeType a = SomeFunction(looooooooooooooooooooooongParameterA,\n" > " loooooooooooooooooooooongParameterB);\n" > - "int j = 2;", > + "int j = 2;", > Alignment); > > verifyFormat("template <typename T, typename T_0 = > very_long_type_name_0,\n" > @@ -7692,6 +7691,13 @@ TEST_F(FormatTest, AlignConsecutiveAssig > verifyFormat("int aa = ((1 > 2) ? 3 : 4);\n" > "float b[1][] = {{3.f}};\n", > Alignment); > + verifyFormat("for (int i = 0; i < 1; i++)\n" > + " int x = 1;\n", > + Alignment); > + verifyFormat("for (i = 0; i < 1; i++)\n" > + " x = 1;\n" > + "y = 1;\n", > + Alignment); > } > > TEST_F(FormatTest, AlignConsecutiveDeclarations) { > @@ -7759,7 +7765,57 @@ TEST_F(FormatTest, AlignConsecutiveDecla > "unsigned oneTwoThree = 123;\n" > "int oneTwo = 12;", > Alignment)); > + // Function prototype alignment > + verifyFormat("int a();\n" > + "double b();", > + Alignment); > + verifyFormat("int a(int x);\n" > + "double b();", > + Alignment); > + unsigned OldColumnLimit = Alignment.ColumnLimit; > + // We need to set ColumnLimit to zero, in order to stress nested > alignments, > + // otherwise the function parameters will be re-flowed onto a single line. > + Alignment.ColumnLimit = 0; > + EXPECT_EQ("int a(int x,\n" > + " float y);\n" > + "double b(int x,\n" > + " double y);", > + format("int a(int x,\n" > + " float y);\n" > + "double b(int x,\n" > + " double y);", > + Alignment)); > + // This ensures that function parameters of function declarations are > + // correctly indented when their owning functions are indented. > + // The failure case here is for 'double y' to not be indented enough. > + EXPECT_EQ("double a(int x);\n" > + "int b(int y,\n" > + " double z);", > + format("double a(int x);\n" > + "int b(int y,\n" > + " double z);", > + Alignment)); > + // Set ColumnLimit low so that we induce wrapping immediately after > + // the function name and opening paren. > + Alignment.ColumnLimit = 13; > + verifyFormat("int function(\n" > + " int x,\n" > + " bool y);", > + Alignment); > + Alignment.ColumnLimit = OldColumnLimit; > + // Ensure function pointers don't screw up recursive alignment > + verifyFormat("int a(int x, void (*fp)(int y));\n" > + "double b();", > + Alignment); > Alignment.AlignConsecutiveAssignments = true; > + // Ensure recursive alignment is broken by function braces, so that the > + // "a = 1" does not align with subsequent assignments inside the function > + // body. > + verifyFormat("int func(int a = 1) {\n" > + " int b = 2;\n" > + " int cc = 3;\n" > + "}", > + Alignment); > verifyFormat("float something = 2000;\n" > "double another = 911;\n" > "int i = 1, j = 10;\n" > @@ -7769,6 +7825,28 @@ TEST_F(FormatTest, AlignConsecutiveDecla > verifyFormat("int oneTwoThree = {0}; // comment\n" > "unsigned oneTwo = 0; // comment", > Alignment); > + // Make sure that scope is correctly tracked, in the absence of braces > + verifyFormat("for (int i = 0; i < n; i++)\n" > + " j = i;\n" > + "double x = 1;\n", > + Alignment); > + verifyFormat("if (int i = 0)\n" > + " j = i;\n" > + "double x = 1;\n", > + Alignment); > + // Ensure operator[] and operator() are comprehended > + verifyFormat("struct test {\n" > + " long long int foo();\n" > + " int operator[](int a);\n" > + " double bar();\n" > + "};\n", > + Alignment); > + verifyFormat("struct test {\n" > + " long long int foo();\n" > + " int operator()(int a);\n" > + " double bar();\n" > + "};\n", > + Alignment); > EXPECT_EQ("void SomeFunction(int parameter = 0) {\n" > " int const i = 1;\n" > " int * j = 2;\n" > @@ -7870,17 +7948,16 @@ TEST_F(FormatTest, AlignConsecutiveDecla > Alignment); > Alignment.AlignConsecutiveAssignments = false; > > - // FIXME: Should align all three declarations > verifyFormat( > "int i = 1;\n" > "SomeType a = SomeFunction(looooooooooooooooooooooongParameterA,\n" > " loooooooooooooooooooooongParameterB);\n" > - "int j = 2;", > + "int j = 2;", > Alignment); > > // Test interactions with ColumnLimit and AlignConsecutiveAssignments: > // We expect declarations and assignments to align, as long as it doesn't > - // exceed the column limit, starting a new alignemnt sequence whenever it > + // exceed the column limit, starting a new alignment sequence whenever it > // happens. > Alignment.AlignConsecutiveAssignments = true; > Alignment.ColumnLimit = 30; > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
