Author: mydeveloperday Date: 2020-05-13T18:31:51+01:00 New Revision: b2eb439317576ce718193763c12bff9fccdfc166
URL: https://github.com/llvm/llvm-project/commit/b2eb439317576ce718193763c12bff9fccdfc166 DIFF: https://github.com/llvm/llvm-project/commit/b2eb439317576ce718193763c12bff9fccdfc166.diff LOG: [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 Added: Modified: 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 Removed: ################################################################################ diff --git a/clang/lib/Format/FormatToken.h b/clang/lib/Format/FormatToken.h index 7b6b1e8ddce7..2ad839a6a4f0 100644 --- a/clang/lib/Format/FormatToken.h +++ b/clang/lib/Format/FormatToken.h @@ -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 diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp index 084b4fbb0bcf..b303758f1cb8 100644 --- a/clang/lib/Format/UnwrappedLineParser.cpp +++ b/clang/lib/Format/UnwrappedLineParser.cpp @@ -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) && @@ -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; } } diff --git a/clang/lib/Format/WhitespaceManager.cpp b/clang/lib/Format/WhitespaceManager.cpp index bc71a89fc92b..e641f10ccead 100644 --- a/clang/lib/Format/WhitespaceManager.cpp +++ b/clang/lib/Format/WhitespaceManager.cpp @@ -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; @@ -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; diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp index 227e12dfaf73..430423fbf941 100644 --- a/clang/unittests/Format/FormatTest.cpp +++ b/clang/unittests/Format/FormatTest.cpp @@ -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;", diff --git a/clang/unittests/Format/FormatTestComments.cpp b/clang/unittests/Format/FormatTestComments.cpp index d5b9f8e0885a..47509f29744c 100644 --- a/clang/unittests/Format/FormatTestComments.cpp +++ b/clang/unittests/Format/FormatTestComments.cpp @@ -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) { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits