r298574 - Fix issues in clang-format's AlignConsecutive modes.
Nikola Smiljanic via cfe-commits
cfe-commits at lists.llvm.org
Wed Mar 22 19:51:26 PDT 2017
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;
More information about the cfe-commits
mailing list