Author: r4nt Date: 2026-10-21T20:55:34+02:00 New Revision: e6602770fcd6d8d12be1f2e7df082a0fdfc4d1bf
URL: https://github.com/llvm/llvm-project/commit/e6602770fcd6d8d12be1f2e7df082a0fdfc4d1bf DIFF: https://github.com/llvm/llvm-project/commit/e6602770fcd6d8d12be1f2e7df082a0fdfc4d1bf.diff LOG: [clang-format] Defer line merging to the unexpanded pass (#233944) Defer line merging to the unexpanded pass when expanding macros. When Configured Macro Expansion (Style.Macros) is used, clang-format runs two formatting passes: Run 0 formats the macro-expanded UnwrappedLines (and finalizes all non-macro-expanded lines and macro argument tokens), and Run 1 formats the reconstructed unexpanded UnwrappedLines. Previously, LineJoiner ran in both Run 0 and Run 1, and Run 0 finalized the first token of every top-level UnwrappedLine. This caused several bugs: 1. If a macro expanded to multiple UnwrappedLines whose first line could merge with a preceding header in Run 0 (e.g. `void f() { IF_ERROR_RETURN(x); }` where `IF_ERROR_RETURN(x)` expands to `if (x)\n return x;` with `AllowShortIfStatementsOnASingleLine: Never` and `AllowShortFunctionsOnASingleLine: All`, or `FN=class C { int f` in `void f() { FN(a *b); }; }`), Run 0 merged the first expanded line onto `void f() {` and finalized the macro token's whitespace, while leaving `}` on a subsequent line. 2. Conversely, if a macro call's expanded line was longer than the unexpanded macro call (preventing a merge in Run 0), finalizing the first token of each top-level line in Run 0 prevented LineJoiner in Run 1 from merging lines that fit on a single line in the unexpanded code (or conversely, Run 0 merged lines whose unexpanded macro call exceeded the column limit in Run 1). 3. In MacroCallReconstructor::finalize(), multi-line object-like macros where subsequent expanded lines contained only hidden macro tokens (e.g. `STMTS=f(); g();`) produced empty child ReconstructedLines, crashing on `Last->Children.back()->Tokens.back()` when `Tokens` was empty. Fix this by: - Skipping line merging (`LineJoiner`) and not finalizing the first token of top-level UnwrappedLines during Run 0 (`InMacroExpansion`), while still transitioning `MR_ExpandedArg` to `MR_UnexpandedArg` if the first token is a macro argument. - Running `LineJoiner` in Run 1 using the unexpanded token lengths, and disallowing merges across lines that contain multi-line macro expansions or internal line breaks from Run 0. - Skipping empty child lines in `MacroCallReconstructor::finalize()` after marking `Last->Tok->MacroParent = true`, and marking `Reconstructed`'s last token as `MacroParent` whenever a macro call expands to multiple `UnwrappedLine`s. Added: Modified: clang/lib/Format/MacroCallReconstructor.cpp clang/lib/Format/TokenAnnotator.h clang/lib/Format/UnwrappedLineFormatter.cpp clang/lib/Format/UnwrappedLineParser.cpp clang/lib/Format/UnwrappedLineParser.h clang/lib/Format/WhitespaceManager.cpp clang/lib/Format/WhitespaceManager.h clang/unittests/Format/FormatTestMacroExpansion.cpp Removed: ################################################################################ diff --git a/clang/lib/Format/MacroCallReconstructor.cpp b/clang/lib/Format/MacroCallReconstructor.cpp index 895d9f93dfce3..f62e208043e89 100644 --- a/clang/lib/Format/MacroCallReconstructor.cpp +++ b/clang/lib/Format/MacroCallReconstructor.cpp @@ -484,12 +484,13 @@ void MacroCallReconstructor::finalize() { LineNode *Last = (*I)->Tokens.back().get(); ++I; for (auto *E = Top.Children.end(); I != E; ++I) { - assert(Last->Children.empty()); - Last->Children.push_back(std::move(*I)); - // Mark the previous line's last token as generated by a macro expansion // so the formatting algorithm can take that into account. Last->Tok->MacroParent = true; + if ((*I)->Tokens.empty()) + continue; + assert(Last->Children.empty()); + Last->Children.push_back(std::move(*I)); Last = Last->Children.back()->Tokens.back().get(); } diff --git a/clang/lib/Format/TokenAnnotator.h b/clang/lib/Format/TokenAnnotator.h index c909c39e92ea3..57d479a99f92b 100644 --- a/clang/lib/Format/TokenAnnotator.h +++ b/clang/lib/Format/TokenAnnotator.h @@ -57,7 +57,7 @@ class AnnotatedLine { MatchingClosingBlockLineIndex(Line.MatchingClosingBlockLineIndex), InPPDirective(Line.InPPDirective), InPragmaDirective(Line.InPragmaDirective), - InMacroBody(Line.InMacroBody), + InMacroBody(Line.InMacroBody), InMacroExpansion(Line.InMacroExpansion), IsModuleOrImportDecl(Line.IsModuleOrImportDecl), MustBeDeclaration(Line.MustBeDeclaration), MightBeFunctionDecl(false), IsMultiVariableDeclStmt(false), Affected(false), @@ -70,6 +70,8 @@ class AnnotatedLine { // Next and Previous for every token, as previous formatting runs might have // left them in a diff erent state. First->Previous = nullptr; + if (First->MacroParent) + ContainsMacroCall = true; FormatToken *Current = First; addChildren(Line.Tokens.front(), Current); for (const UnwrappedLineNode &Node : llvm::drop_begin(Line.Tokens)) { @@ -180,6 +182,7 @@ class AnnotatedLine { bool InPPDirective; bool InPragmaDirective; bool InMacroBody; + bool InMacroExpansion; bool IsModuleOrImportDecl; bool MustBeDeclaration; bool MightBeFunctionDecl; diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp index e42a2083f7268..304d8649f2e81 100644 --- a/clang/lib/Format/UnwrappedLineFormatter.cpp +++ b/clang/lib/Format/UnwrappedLineFormatter.cpp @@ -215,7 +215,10 @@ class LineJoiner { return nullptr; const AnnotatedLine *Current = *Next; IndentTracker.nextLine(*Current); - unsigned MergedLines = tryFitMultipleLinesInOne(IndentTracker, Next, End); + unsigned MergedLines = + Current->InMacroExpansion + ? 0 + : tryFitMultipleLinesInOne(IndentTracker, Next, End); if (MergedLines > 0 && Style.ColumnLimit == 0) { // Disallow line merging if there is a break at the start of one of the // input lines. @@ -223,6 +226,12 @@ class LineJoiner { if (Next[i + 1]->First->NewlinesBefore > 0) MergedLines = 0; } + for (unsigned i = 0; i <= MergedLines; ++i) { + if (cannotMergeMacroLine(Next[i])) { + MergedLines = 0; + break; + } + } if (!DryRun) for (unsigned i = 0; i < MergedLines; ++i) join(*Next[0], *Next[i + 1]); @@ -1109,6 +1118,19 @@ class LineJoiner { return false; } + bool cannotMergeMacroLine(const AnnotatedLine *Line) { + assert(Line->First); + if (Line->Last->MacroParent) + return true; + for (const FormatToken *Tok = Line->First; Tok; Tok = Tok->Next) { + if (Tok->MacroParent && !Tok->Children.empty()) + return true; + if (Tok != Line->First && Tok->getDecision() == FD_Break) + return true; + } + return false; + } + void join(AnnotatedLine &A, const AnnotatedLine &B) { assert(!A.Last->Next); assert(!B.First->Previous); @@ -1134,14 +1156,15 @@ class LineJoiner { const SmallVectorImpl<AnnotatedLine *> &AnnotatedLines; }; -static void markFinalized(FormatToken *Tok) { +static void markFinalized(FormatToken *Tok, bool SkipFirst = false) { if (Tok->is(tok::hash) && !Tok->Previous && Tok->Next && Tok->Next->isOneOf(tok::pp_if, tok::pp_ifdef, tok::pp_ifndef, tok::pp_elif, tok::pp_elifdef, tok::pp_elifndef, tok::pp_else, tok::pp_endif)) { Tok = Tok->Next; + SkipFirst = false; } - for (; Tok; Tok = Tok->Next) { + for (bool First = true; Tok; Tok = Tok->Next, First = false) { if (Tok->MacroCtx && Tok->MacroCtx->Role == MR_ExpandedArg) { // In the first pass we format all macro arguments in the expanded token // stream. Instead of finalizing the macro arguments, we mark that they @@ -1157,7 +1180,7 @@ static void markFinalized(FormatToken *Tok) { Tok->SpacesRequiredBefore = 0; if (!Tok->MustBreakBeforeFinalized) Tok->MustBreakBefore = 0; - } else { + } else if (!First || !SkipFirst) { Tok->Finalized = true; } } @@ -1636,7 +1659,7 @@ unsigned UnwrappedLineFormatter::format( RangeMinLevel = UINT_MAX; } if (!DryRun) - markFinalized(TheLine.First); + markFinalized(TheLine.First, /*SkipFirst=*/TheLine.InMacroExpansion); } PenaltyCache[CacheKey] = Penalty; return Penalty; @@ -1764,8 +1787,11 @@ void UnwrappedLineFormatter::formatFirstToken(const AnnotatedLine &Line, RootToken.NewlinesBefore, Style.KeepEmptyLines.AtEndOfFile ? Style.MaxEmptyLinesToKeep + 1 : 1); unsigned TokenIndent = Newlines ? NewlineIndent : 0; - Whitespaces->replaceWhitespace(RootToken, Newlines, TokenIndent, - TokenIndent); + Whitespaces->replaceWhitespace( + RootToken, Newlines, TokenIndent, TokenIndent, /*AlignedTo=*/nullptr, + /*InPPDirective=*/false, + /*IndentedFromColumn=*/0, + /*CreateReplacement=*/!Line.InMacroExpansion); return; } @@ -1787,10 +1813,10 @@ void UnwrappedLineFormatter::formatFirstToken(const AnnotatedLine &Line, Indent = 0; } - Whitespaces->replaceWhitespace(RootToken, RootToken.Newlines, Indent, Indent, - /*AlignedTo=*/nullptr, - Line.InPPDirective && - !RootToken.HasUnescapedNewline); + Whitespaces->replaceWhitespace( + RootToken, RootToken.Newlines, Indent, Indent, /*AlignedTo=*/nullptr, + Line.InPPDirective && !RootToken.HasUnescapedNewline, + /*IndentedFromColumn=*/0, /*CreateReplacement=*/!Line.InMacroExpansion); } unsigned diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp index 72ebb8f75120f..f0531d2c8a9df 100644 --- a/clang/lib/Format/UnwrappedLineParser.cpp +++ b/clang/lib/Format/UnwrappedLineParser.cpp @@ -230,19 +230,22 @@ void UnwrappedLineParser::parse() { // replaced by the expansion. if (!ExpandedLines.empty()) { LLVM_DEBUG(llvm::dbgs() << "Expanded lines:\n"); - for (const auto &Line : Lines) { + for (auto &Line : Lines) { if (!Line.Tokens.empty()) { auto it = ExpandedLines.find(Line.Tokens.begin()->Tok); if (it != ExpandedLines.end()) { - for (const auto &Expanded : it->second) { + for (auto &Expanded : it->second) { + Expanded.InMacroExpansion = true; LLVM_DEBUG(printDebugInfo(Expanded)); Callback.consumeUnwrappedLine(Expanded); } continue; } } + Line.InMacroExpansion = true; LLVM_DEBUG(printDebugInfo(Line)); Callback.consumeUnwrappedLine(Line); + Line.InMacroExpansion = false; } Callback.finishRun(); } @@ -4854,6 +4857,8 @@ void UnwrappedLineParser::addUnwrappedLine(LineLevel AdjustLevel) { llvm::dbgs() << "Adding unexpanded line:\n"; printDebugInfo(Reconstructed); }); + if (CurrentExpandedLines.size() > 1) + Reconstructed.Tokens.back().Tok->MacroParent = true; ExpandedLines[Reconstructed.Tokens.begin()->Tok] = CurrentExpandedLines; Lines.push_back(std::move(Reconstructed)); CurrentExpandedLines.clear(); diff --git a/clang/lib/Format/UnwrappedLineParser.h b/clang/lib/Format/UnwrappedLineParser.h index 48d8eaabef1bd..4a47f8158a0fe 100644 --- a/clang/lib/Format/UnwrappedLineParser.h +++ b/clang/lib/Format/UnwrappedLineParser.h @@ -49,6 +49,9 @@ struct UnwrappedLine { bool InPragmaDirective = false; /// Whether it is part of a macro body. bool InMacroBody = false; + /// Whether this \c UnwrappedLine is part of the first pass of a multi-pass + /// macro expansion run. + bool InMacroExpansion = false; /// Nesting level of unbraced body of a control statement. unsigned UnbracedBodyLevel = 0; diff --git a/clang/lib/Format/WhitespaceManager.cpp b/clang/lib/Format/WhitespaceManager.cpp index cd2feb3a5964b..314400a7fd45e 100644 --- a/clang/lib/Format/WhitespaceManager.cpp +++ b/clang/lib/Format/WhitespaceManager.cpp @@ -67,19 +67,17 @@ WhitespaceManager::Change::Change( StartOfBlockComment(nullptr), IndentationOffset(0), ConditionalsLevel(0) { } -void WhitespaceManager::replaceWhitespace(FormatToken &Tok, unsigned Newlines, - unsigned Spaces, - unsigned StartOfTokenColumn, - const FormatToken *AlignedTo, - bool InPPDirective, - unsigned IndentedFromColumn) { +void WhitespaceManager::replaceWhitespace( + FormatToken &Tok, unsigned Newlines, unsigned Spaces, + unsigned StartOfTokenColumn, const FormatToken *AlignedTo, + bool InPPDirective, unsigned IndentedFromColumn, bool CreateReplacement) { if (Tok.Finalized || (Tok.MacroCtx && Tok.MacroCtx->Role == MR_ExpandedArg)) return; - Tok.setDecision((Newlines > 0) ? FD_Break : FD_Continue); - Changes.push_back(Change(Tok, /*CreateReplacement=*/true, Tok.WhitespaceRange, - Spaces, StartOfTokenColumn, IndentedFromColumn, - Newlines, "", "", AlignedTo, - InPPDirective && !Tok.IsFirst, + if (CreateReplacement) + Tok.setDecision((Newlines > 0) ? FD_Break : FD_Continue); + Changes.push_back(Change(Tok, CreateReplacement, Tok.WhitespaceRange, Spaces, + StartOfTokenColumn, IndentedFromColumn, Newlines, "", + "", AlignedTo, InPPDirective && !Tok.IsFirst, /*IsInsideToken=*/false)); } diff --git a/clang/lib/Format/WhitespaceManager.h b/clang/lib/Format/WhitespaceManager.h index 66a064ae81dbf..6174e4961d763 100644 --- a/clang/lib/Format/WhitespaceManager.h +++ b/clang/lib/Format/WhitespaceManager.h @@ -58,7 +58,8 @@ class WhitespaceManager { unsigned StartOfTokenColumn, const FormatToken *AlignedTo = nullptr, bool InPPDirective = false, - unsigned IndentedFromColumn = 0); + unsigned IndentedFromColumn = 0, + bool CreateReplacement = true); /// Adds information about an unchangeable token's whitespace. /// diff --git a/clang/unittests/Format/FormatTestMacroExpansion.cpp b/clang/unittests/Format/FormatTestMacroExpansion.cpp index ca8a5180c32d5..7bc9cc98f4e24 100644 --- a/clang/unittests/Format/FormatTestMacroExpansion.cpp +++ b/clang/unittests/Format/FormatTestMacroExpansion.cpp @@ -230,12 +230,67 @@ a))", TEST_F(FormatTestMacroExpansion, KeepParensWhenExpandingObjectLikeMacros) { FormatStyle Style = getLLVMStyle(); Style.Macros.push_back("FN=class C { int f"); - verifyFormat("void f() { FN(a *b);\n" + verifyFormat("void f() {\n" + " FN(a *b);\n" " };\n" "}", Style); } +TEST_F(FormatTestMacroExpansion, ShortFunctionsAndBlocksWithMacroExpansion) { + FormatStyle Style = getLLVMStyleWithColumns(35); + Style.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_WithoutElse; + Style.Macros = { + "ID(x)=x", + "VERY_LONG_MACRO_NAME(x)=x", + "EXPANDS_LONG(x)=very_long_function_call(x)", + "IF_ERROR_RETURN(x)=if (x) return x", + "STMTS=f(); g();", + "STMTS_NO_SEMI=f(); g()", + }; + + verifyFormat("void f() { ID(a *b); }", Style); + verifyFormat("void f() { int x = ID(a * b); }", Style); + verifyFormat("void f() {\n" + " VERY_LONG_MACRO_NAME(a *b);\n" + "}", + Style); + verifyFormat("void f() {\n" + " int x = VERY_LONG_MACRO_NAME(x);\n" + "}", + Style); + verifyFormat("void f() {\n" + " EXPANDS_LONG(a * b) +\n" + " c;\n" + "}", + Style); + verifyFormat("void f() {\n" + " IF_ERROR_RETURN(x);\n" + "}", + Style); + verifyFormat("void f() {\n" + " IF_ERROR_RETURN(a * b);\n" + "}", + Style); + verifyFormat("void f() {\n" + " STMTS\n" + "}", + Style); + verifyFormat("void f() {\n" + " STMTS_NO_SEMI;\n" + "}", + Style); + + verifyFormat("void f() {\n" + " if (a) ID(b *c);\n" + " if (a)\n" + " VERY_LONG_MACRO_NAME(b *c);\n" + " if (a)\n" + " IF_ERROR_RETURN(b * c);\n" + "}", + Style); +} + TEST_F(FormatTestMacroExpansion, DoesNotExpandFunctionLikeMacrosWithoutParens) { FormatStyle Style = getLLVMStyle(); Style.Macros.push_back("CLASS()=class C {"); _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
