This caused https://bugs.llvm.org/show_bug.cgi?id=33507 which is one of the last release blockers for 5.0.0.
Can someone who's familiar with this code please take a look? On Thu, Aug 24, 2017 at 11:12 AM, Hans Wennborg <h...@chromium.org> wrote: > 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
