JakeMerdichAMD created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Currently the 'AlignConsecutive*' options incorrectly align across
elif and else statements, even if they are very far away and across
unrelated preprocessor macros.
This failed since on preprocessor run 2+, there is not enough context
about the #ifdefs to actually differentiate one block from another,
causing them to align across different blocks or even large sections of
the file.
Eg, with AlignConsecutiveAssignments:
\#if FOO // Run 1
\#else // Run 1
int a = 1; // Run 2, wrong
\#endif // Run 1
\#if FOO // Run 1
\#else // Run 1
int bar = 1; // Run 2
\#endif // Run 1
is read as
int a = 1; // Run 2, wrong
int bar = 1; // Run 2
The approach taken to fix this was to add a new flag to Token that
forces breaking alignment across groups of lines (MustBreakAlignBefore)
in a similar manner to the existing flag that forces a line break
(MustBreakBefore). This flag is set for the first Token after a
preprocessor statement or diff conflict marker.
Possible alternatives might be hashing preprocessor state to detect if
two lines come from the same block (but the way that ifdefs are
sometimes deferred makes that tricky) or showing the preprocessor
statements on all passes instead of just the first one (seems harder).
Fixes #25167,#31281
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D79388
Files:
clang/lib/Format/FormatToken.h
clang/lib/Format/UnwrappedLineParser.cpp
clang/lib/Format/WhitespaceManager.cpp
clang/unittests/Format/FormatTest.cpp
clang/unittests/Format/FormatTestComments.cpp
Index: clang/unittests/Format/FormatTestComments.cpp
===================================================================
--- clang/unittests/Format/FormatTestComments.cpp
+++ clang/unittests/Format/FormatTestComments.cpp
@@ -2780,6 +2780,27 @@
" // line 2 about b\n"
" long b;",
getLLVMStyleWithColumns(80)));
+
+ // Checks an edge case in preprocessor handling.
+ // These comments should *not* be aligned
+ EXPECT_EQ(
+ "#if FOO\n"
+ "#else\n"
+ "long a; // Line about a\n"
+ "#endif\n"
+ "#if BAR\n"
+ "#else\n"
+ "long b_long_name; // Line about b\n"
+ "#endif\n",
+ format("#if FOO\n"
+ "#else\n"
+ "long a; // Line about a\n" // Previous (bad) behavior
+ "#endif\n"
+ "#if BAR\n"
+ "#else\n"
+ "long b_long_name; // Line about b\n"
+ "#endif\n",
+ getLLVMStyleWithColumns(80)));
}
TEST_F(FormatTestComments, AlignsBlockCommentDecorations) {
Index: clang/unittests/Format/FormatTest.cpp
===================================================================
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -11429,6 +11429,29 @@
verifyFormat("int oneTwoThree = 123; // comment\n"
"int oneTwo = 12; // comment",
Alignment);
+
+ // Bug 25167
+ verifyFormat("#if A\n"
+ "#else\n"
+ "int aaaaaaaa = 12;\n"
+ "#endif\n"
+ "#if B\n"
+ "#else\n"
+ "int a = 12;\n"
+ "#endif\n",
+ Alignment);
+ verifyFormat("enum foo {\n"
+ "#if A\n"
+ "#else\n"
+ " aaaaaaaa = 12;\n"
+ "#endif\n"
+ "#if B\n"
+ "#else\n"
+ " a = 12;\n"
+ "#endif\n"
+ "};\n",
+ Alignment);
+
EXPECT_EQ("int a = 5;\n"
"\n"
"int oneTwoThree = 123;",
Index: clang/lib/Format/WhitespaceManager.cpp
===================================================================
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -377,9 +377,11 @@
if (Changes[i].NewlinesBefore != 0) {
CommasBeforeMatch = 0;
EndOfSequence = i;
- // If there is a blank line, or if the last line didn't contain any
- // matching token, the sequence ends here.
- if (Changes[i].NewlinesBefore > 1 || !FoundMatchOnLine)
+ // If there is a blank line, there is a forced-align-break (eg,
+ // preprocessor), or if the last line didn't contain any matching token,
+ // the sequence ends here.
+ if (Changes[i].NewlinesBefore > 1 ||
+ Changes[i].Tok->MustBreakAlignBefore || !FoundMatchOnLine)
AlignCurrentSequence();
FoundMatchOnLine = false;
@@ -618,6 +620,8 @@
if (Changes[i].StartOfBlockComment)
continue;
Newlines += Changes[i].NewlinesBefore;
+ if (Changes[i].Tok->MustBreakAlignBefore)
+ BreakBeforeNext = true;
if (!Changes[i].IsTrailingComment)
continue;
Index: clang/lib/Format/UnwrappedLineParser.cpp
===================================================================
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2959,6 +2959,7 @@
}
FormatTok = Tokens->getNextToken();
FormatTok->MustBreakBefore = true;
+ FormatTok->MustBreakAlignBefore = true;
}
if (!PPStack.empty() && (PPStack.back().Kind == PP_Unreachable) &&
@@ -2983,6 +2984,7 @@
Line->Tokens.push_back(UnwrappedLineNode(Tok));
if (MustBreakBeforeNextToken) {
Line->Tokens.back().Tok->MustBreakBefore = true;
+ Line->Tokens.back().Tok->MustBreakAlignBefore = true;
MustBreakBeforeNextToken = false;
}
}
Index: clang/lib/Format/FormatToken.h
===================================================================
--- clang/lib/Format/FormatToken.h
+++ clang/lib/Format/FormatToken.h
@@ -180,6 +180,12 @@
/// before the token.
bool MustBreakBefore = false;
+ /// Whether to not align across this token
+ ///
+ /// This happens for example when a preprocessor directive ended directly
+ /// before the token, but very rarely otherwise.
+ bool MustBreakAlignBefore = false;
+
/// The raw text of the token.
///
/// Contains the raw token text without leading whitespace and without leading
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
