Skip to content

Commit b2eb439

Browse files
[clang-format] Fix AlignConsecutive on PP blocks
Summary: 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. Fixes #25167,#31281 Patch By: JakeMerdichAMD Reviewed By: MyDeveloperDay Tags: #clang, #clang-format Differential Revision: https://reviews.llvm.org/D79388
1 parent 0ab3ba2 commit b2eb439

File tree

5 files changed

+59
-3
lines changed

5 files changed

+59
-3
lines changed

clang/lib/Format/FormatToken.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,12 @@ struct FormatToken {
182182
/// before the token.
183183
bool MustBreakBefore = false;
184184

185+
/// Whether to not align across this token
186+
///
187+
/// This happens for example when a preprocessor directive ended directly
188+
/// before the token, but very rarely otherwise.
189+
bool MustBreakAlignBefore = false;
190+
185191
/// The raw text of the token.
186192
///
187193
/// Contains the raw token text without leading whitespace and without leading

clang/lib/Format/UnwrappedLineParser.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2968,6 +2968,7 @@ void UnwrappedLineParser::readToken(int LevelDifference) {
29682968
}
29692969
FormatTok = Tokens->getNextToken();
29702970
FormatTok->MustBreakBefore = true;
2971+
FormatTok->MustBreakAlignBefore = true;
29712972
}
29722973

29732974
if (!PPStack.empty() && (PPStack.back().Kind == PP_Unreachable) &&
@@ -2992,6 +2993,7 @@ void UnwrappedLineParser::pushToken(FormatToken *Tok) {
29922993
Line->Tokens.push_back(UnwrappedLineNode(Tok));
29932994
if (MustBreakBeforeNextToken) {
29942995
Line->Tokens.back().Tok->MustBreakBefore = true;
2996+
Line->Tokens.back().Tok->MustBreakAlignBefore = true;
29952997
MustBreakBeforeNextToken = false;
29962998
}
29972999
}

clang/lib/Format/WhitespaceManager.cpp

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -377,9 +377,11 @@ static unsigned AlignTokens(const FormatStyle &Style, F &&Matches,
377377
if (Changes[i].NewlinesBefore != 0) {
378378
CommasBeforeMatch = 0;
379379
EndOfSequence = i;
380-
// If there is a blank line, or if the last line didn't contain any
381-
// matching token, the sequence ends here.
382-
if (Changes[i].NewlinesBefore > 1 || !FoundMatchOnLine)
380+
// If there is a blank line, there is a forced-align-break (eg,
381+
// preprocessor), or if the last line didn't contain any matching token,
382+
// the sequence ends here.
383+
if (Changes[i].NewlinesBefore > 1 ||
384+
Changes[i].Tok->MustBreakAlignBefore || !FoundMatchOnLine)
383385
AlignCurrentSequence();
384386

385387
FoundMatchOnLine = false;
@@ -618,6 +620,8 @@ void WhitespaceManager::alignTrailingComments() {
618620
if (Changes[i].StartOfBlockComment)
619621
continue;
620622
Newlines += Changes[i].NewlinesBefore;
623+
if (Changes[i].Tok->MustBreakAlignBefore)
624+
BreakBeforeNext = true;
621625
if (!Changes[i].IsTrailingComment)
622626
continue;
623627

clang/unittests/Format/FormatTest.cpp

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11457,6 +11457,29 @@ TEST_F(FormatTest, AlignConsecutiveAssignments) {
1145711457
verifyFormat("int oneTwoThree = 123; // comment\n"
1145811458
"int oneTwo = 12; // comment",
1145911459
Alignment);
11460+
11461+
// Bug 25167
11462+
verifyFormat("#if A\n"
11463+
"#else\n"
11464+
"int aaaaaaaa = 12;\n"
11465+
"#endif\n"
11466+
"#if B\n"
11467+
"#else\n"
11468+
"int a = 12;\n"
11469+
"#endif\n",
11470+
Alignment);
11471+
verifyFormat("enum foo {\n"
11472+
"#if A\n"
11473+
"#else\n"
11474+
" aaaaaaaa = 12;\n"
11475+
"#endif\n"
11476+
"#if B\n"
11477+
"#else\n"
11478+
" a = 12;\n"
11479+
"#endif\n"
11480+
"};\n",
11481+
Alignment);
11482+
1146011483
EXPECT_EQ("int a = 5;\n"
1146111484
"\n"
1146211485
"int oneTwoThree = 123;",

clang/unittests/Format/FormatTestComments.cpp

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2780,6 +2780,27 @@ TEST_F(FormatTestComments, AlignTrailingComments) {
27802780
" // line 2 about b\n"
27812781
" long b;",
27822782
getLLVMStyleWithColumns(80)));
2783+
2784+
// Checks an edge case in preprocessor handling.
2785+
// These comments should *not* be aligned
2786+
EXPECT_EQ(
2787+
"#if FOO\n"
2788+
"#else\n"
2789+
"long a; // Line about a\n"
2790+
"#endif\n"
2791+
"#if BAR\n"
2792+
"#else\n"
2793+
"long b_long_name; // Line about b\n"
2794+
"#endif\n",
2795+
format("#if FOO\n"
2796+
"#else\n"
2797+
"long a; // Line about a\n" // Previous (bad) behavior
2798+
"#endif\n"
2799+
"#if BAR\n"
2800+
"#else\n"
2801+
"long b_long_name; // Line about b\n"
2802+
"#endif\n",
2803+
getLLVMStyleWithColumns(80)));
27832804
}
27842805

27852806
TEST_F(FormatTestComments, AlignsBlockCommentDecorations) {

0 commit comments

Comments
 (0)