Skip to content

Commit

Permalink
[clang-format] Fix AlignConsecutive on PP blocks
Browse files Browse the repository at this point in the history
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
  • Loading branch information
mydeveloperday committed May 13, 2020
1 parent 0ab3ba2 commit b2eb439
Show file tree
Hide file tree
Showing 5 changed files with 59 additions and 3 deletions.
6 changes: 6 additions & 0 deletions clang/lib/Format/FormatToken.h
Expand Up @@ -182,6 +182,12 @@ struct FormatToken {
/// 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
Expand Down
2 changes: 2 additions & 0 deletions clang/lib/Format/UnwrappedLineParser.cpp
Expand Up @@ -2968,6 +2968,7 @@ void UnwrappedLineParser::readToken(int LevelDifference) {
}
FormatTok = Tokens->getNextToken();
FormatTok->MustBreakBefore = true;
FormatTok->MustBreakAlignBefore = true;
}

if (!PPStack.empty() && (PPStack.back().Kind == PP_Unreachable) &&
Expand All @@ -2992,6 +2993,7 @@ void UnwrappedLineParser::pushToken(FormatToken *Tok) {
Line->Tokens.push_back(UnwrappedLineNode(Tok));
if (MustBreakBeforeNextToken) {
Line->Tokens.back().Tok->MustBreakBefore = true;
Line->Tokens.back().Tok->MustBreakAlignBefore = true;
MustBreakBeforeNextToken = false;
}
}
Expand Down
10 changes: 7 additions & 3 deletions clang/lib/Format/WhitespaceManager.cpp
Expand Up @@ -377,9 +377,11 @@ static unsigned AlignTokens(const FormatStyle &Style, F &&Matches,
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;
Expand Down Expand Up @@ -618,6 +620,8 @@ void WhitespaceManager::alignTrailingComments() {
if (Changes[i].StartOfBlockComment)
continue;
Newlines += Changes[i].NewlinesBefore;
if (Changes[i].Tok->MustBreakAlignBefore)
BreakBeforeNext = true;
if (!Changes[i].IsTrailingComment)
continue;

Expand Down
23 changes: 23 additions & 0 deletions clang/unittests/Format/FormatTest.cpp
Expand Up @@ -11457,6 +11457,29 @@ TEST_F(FormatTest, AlignConsecutiveAssignments) {
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;",
Expand Down
21 changes: 21 additions & 0 deletions clang/unittests/Format/FormatTestComments.cpp
Expand Up @@ -2780,6 +2780,27 @@ TEST_F(FormatTestComments, AlignTrailingComments) {
" // 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) {
Expand Down

0 comments on commit b2eb439

Please sign in to comment.