https://github.com/r4nt updated https://github.com/llvm/llvm-project/pull/233758
>From 611804c5de0723c37f25282fae9c30f6adb40120 Mon Sep 17 00:00:00 2001 From: Manuel Klimek <[email protected]> Date: Mon, 19 Oct 2026 14:21:13 +0200 Subject: [PATCH] [clang-format] Fix child line formatting and indentation in macro expansion Fix three issues when expanding configured macros (`Style.Macros`): 1. Avoid formatting child lines of reconstructed macro calls twice (Fixes #78263): `UnwrappedLineFormatter::format` formats `Line.Children` of tokens in a line. When formatting the reconstructed unexpanded macro call line, its arguments' child lines (such as a block inside `PACK(struct Foo { ... })` or a raw string literal inside `ASSIGN_OR_RETURN(...)`) were already formatted during the expanded macro pass (`MR_ExpandedArg`), or vice versa. Formatting the macro argument's `Children` during the `MR_ExpandedArg` pass caused duplicate whitespace replacements (crashing in `WhitespaceManager` when `SeparateDefinitionBlocks` is enabled) and caused `ContinuationIndenter::reformatRawStringLiteral` to use the expanded pass's column indentation instead of the reconstructed macro call's indentation. Skip formatting `Children` of `MR_ExpandedArg` tokens during the expanded pass and format `Children` of `MR_UnexpandedArg` tokens during the reconstructed pass, passing the parent line's `DryRun` state down into recursive `LineFormatter->format` calls. 2. Annotate and analyze each run in `TokenAnalyzer::finishRun()` (Fixes #233300): Previously, `UnwrappedLineParser::parse()` ran all `#if`/`#else` branch passes before `TokenAnalyzer::process()` annotated any run. Because `FormatToken` objects are shared across branches, macro reconstruction in one branch left `MacroCtx` set on shared tokens, causing a subsequent branch to re-reconstruct tokens with finalized types (`FormatToken::setType` assertion failure). Instead, annotate and analyze each run inside `TokenAnalyzer::finishRun()` as soon as the parser finishes that branch, and reset `Token->MacroCtx` in `UnwrappedLineParser::reset()` before parsing the next branch. 3. Guard against empty reconstructed macro lines on syntax errors: In `UnwrappedLineParser::addUnwrappedLine()`, check `!Line->Tokens.empty()` after `Reconstruct->takeResult()` so malformed macro calls that reconstruct to an empty line do not dereference an empty token list. Fixes #78263 Fixes #233300 --- clang/lib/Format/ContinuationIndenter.cpp | 3 +- clang/lib/Format/TokenAnalyzer.cpp | 36 +++--- clang/lib/Format/TokenAnalyzer.h | 3 +- clang/lib/Format/UnwrappedLineFormatter.cpp | 4 +- clang/lib/Format/UnwrappedLineParser.cpp | 18 ++- .../Format/FormatTestMacroExpansion.cpp | 113 ++++++++++++++++++ 6 files changed, 153 insertions(+), 24 deletions(-) diff --git a/clang/lib/Format/ContinuationIndenter.cpp b/clang/lib/Format/ContinuationIndenter.cpp index bb8179cbc4f95f..fd950201578475 100644 --- a/clang/lib/Format/ContinuationIndenter.cpp +++ b/clang/lib/Format/ContinuationIndenter.cpp @@ -2557,7 +2557,8 @@ unsigned ContinuationIndenter::handleEndOfLine(const FormatToken &Current, // Compute the raw string style to use in case this is a raw string literal // that can be reformatted. auto RawStringStyle = getRawStringStyle(Current, State); - if (RawStringStyle && !Current.Finalized) { + if (RawStringStyle && !Current.Finalized && + (!Current.MacroCtx || Current.MacroCtx->Role != MR_ExpandedArg)) { Penalty = reformatRawStringLiteral(Current, State, *RawStringStyle, DryRun, Newline); } else if (Current.IsMultiline && Current.isNot(TT_BlockComment)) { diff --git a/clang/lib/Format/TokenAnalyzer.cpp b/clang/lib/Format/TokenAnalyzer.cpp index 6ba3b053126075..73e567ce115383 100644 --- a/clang/lib/Format/TokenAnalyzer.cpp +++ b/clang/lib/Format/TokenAnalyzer.cpp @@ -83,7 +83,6 @@ Environment::Environment(StringRef Code, StringRef FileName, TokenAnalyzer::TokenAnalyzer(const Environment &Env, const FormatStyle &Style) : Style(Style), LangOpts(getFormattingLangOpts(Style)), Env(Env), AffectedRangeMgr(Env.getSourceManager(), Env.getCharRanges()), - UnwrappedLines(1), Encoding(encoding::detectEncoding( Env.getSourceManager().getBufferData(Env.getFileID()))) { LLVM_DEBUG( @@ -104,20 +103,19 @@ TokenAnalyzer::process(bool SkipAnnotation) { IdentTable); ArrayRef<FormatToken *> Toks(Lex.lex()); SmallVector<FormatToken *, 10> Tokens(Toks); - UnwrappedLineParser Parser(Env.getSourceManager(), Style, Lex.getKeywords(), - Env.getFirstStartColumn(), Tokens, *this, - Allocator, IdentTable); - Parser.parse(); - assert(UnwrappedLines.back().empty()); + unsigned Penalty = 0; - for (unsigned Run = 0, RunE = UnwrappedLines.size(); Run + 1 != RunE; ++Run) { - const auto &Lines = UnwrappedLines[Run]; + unsigned Run = 0; + bool HasError = false; + OnFinishRun = [&]() { + if (HasError) + return; LLVM_DEBUG(llvm::dbgs() << "Run " << Run << "...\n"); SmallVector<AnnotatedLine *, 16> AnnotatedLines; - AnnotatedLines.reserve(Lines.size()); + AnnotatedLines.reserve(UnwrappedLines.size()); TokenAnnotator Annotator(Style, Lex.getKeywords()); - for (const UnwrappedLine &Line : Lines) { + for (const UnwrappedLine &Line : UnwrappedLines) { AnnotatedLines.push_back(new AnnotatedLine(Line)); if (!SkipAnnotation) Annotator.annotate(*AnnotatedLines.back()); @@ -133,6 +131,7 @@ TokenAnalyzer::process(bool SkipAnnotation) { }); for (AnnotatedLine *Line : AnnotatedLines) delete Line; + ++Run; Penalty += RunResult.second; for (const auto &R : RunResult.first) { @@ -141,20 +140,27 @@ TokenAnalyzer::process(bool SkipAnnotation) { // Replacements to indicate failure. if (Err) { llvm::errs() << llvm::toString(std::move(Err)) << "\n"; - return {tooling::Replacements(), 0}; + HasError = true; + return; } } - } + }; + UnwrappedLineParser Parser(Env.getSourceManager(), Style, Lex.getKeywords(), + Env.getFirstStartColumn(), Tokens, *this, + Allocator, IdentTable); + Parser.parse(); + if (HasError) + return {tooling::Replacements(), 0}; return {Result, Penalty}; } void TokenAnalyzer::consumeUnwrappedLine(const UnwrappedLine &TheLine) { - assert(!UnwrappedLines.empty()); - UnwrappedLines.back().push_back(TheLine); + UnwrappedLines.push_back(TheLine); } void TokenAnalyzer::finishRun() { - UnwrappedLines.push_back(SmallVector<UnwrappedLine, 16>()); + OnFinishRun(); + UnwrappedLines.clear(); } } // end namespace format diff --git a/clang/lib/Format/TokenAnalyzer.h b/clang/lib/Format/TokenAnalyzer.h index ef559099d325d2..40f95684e10dc0 100644 --- a/clang/lib/Format/TokenAnalyzer.h +++ b/clang/lib/Format/TokenAnalyzer.h @@ -97,7 +97,8 @@ class TokenAnalyzer : public UnwrappedLineConsumer { const Environment &Env; // AffectedRangeMgr stores ranges to be fixed. AffectedRangeManager AffectedRangeMgr; - SmallVector<SmallVector<UnwrappedLine, 16>, 2> UnwrappedLines; + SmallVector<UnwrappedLine, 16> UnwrappedLines; + std::function<void()> OnFinishRun; encoding::Encoding Encoding; }; diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp index e42a2083f7268a..81131669645ccc 100644 --- a/clang/lib/Format/UnwrappedLineFormatter.cpp +++ b/clang/lib/Format/UnwrappedLineFormatter.cpp @@ -1223,13 +1223,15 @@ class LineFormatter { } if (NewLine || Previous.MacroParent) { - const ParenState &P = State.Stack.back(); + ParenState &P = State.Stack.back(); int AdditionalIndent = P.Indent.Total - Previous.Children[0]->Level * Style.IndentWidth; Penalty += BlockFormatter->format(Previous.Children, DryRun, AdditionalIndent, /*FixBadIndentation=*/true); + if (Previous.MacroParent) + P.NestedBlockIndent = P.Indent.Total; return true; } diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp index 901335950b9fac..765f495a00c827 100644 --- a/clang/lib/Format/UnwrappedLineParser.cpp +++ b/clang/lib/Format/UnwrappedLineParser.cpp @@ -812,7 +812,7 @@ FormatToken *UnwrappedLineParser::parseBlock( !parsingPPDirective() ? PreprocessorDirectives.size() : 0; addUnwrappedLine(); size_t OpeningLineIndex = - CurrentLines->empty() + CurrentLines->empty() || !CurrentExpandedLines.empty() ? (UnwrappedLine::kInvalidIndex) : (CurrentLines->size() - 1 - NbPreprocessorDirectives); @@ -873,6 +873,8 @@ FormatToken *UnwrappedLineParser::parseBlock( if (Previous->is(tok::r_brace) && !Previous->Optional) return false; } + if (OpeningLineIndex == UnwrappedLine::kInvalidIndex) + return false; assert(!CurrentLines->empty()); auto &LastLine = CurrentLines->back(); if (LastLine.Level == InitialLevel + 1 && !mightFitOnOneLine(LastLine)) @@ -930,7 +932,8 @@ FormatToken *UnwrappedLineParser::parseBlock( if (MunchSemi && FormatTok->is(tok::semi)) nextToken(); - if (PPStartHash == PPEndHash) { + if (PPStartHash == PPEndHash && CurrentExpandedLines.empty() && + !containsExpansion(*Line)) { Line->MatchingOpeningBlockLineIndex = OpeningLineIndex; if (OpeningLineIndex != UnwrappedLine::kInvalidIndex) { // Update the opening line to add the forward reference as well @@ -4016,9 +4019,10 @@ bool UnwrappedLineParser::parseEnum() { if (!ManageWhitesmithsBraces) ++Line->Level; } - const auto OpeningLineIndex = CurrentLines->empty() - ? UnwrappedLine::kInvalidIndex - : CurrentLines->size() - 1; + const auto OpeningLineIndex = + CurrentLines->empty() || !CurrentExpandedLines.empty() + ? UnwrappedLine::kInvalidIndex + : CurrentLines->size() - 1; bool HasError = !parseBracedList(/*IsAngleBracket=*/false, /*IsEnum=*/true); if (!Style.AllowShortEnumsOnASingleLine && !ManageWhitesmithsBraces) --Line->Level; @@ -4028,8 +4032,10 @@ bool UnwrappedLineParser::parseEnum() { addUnwrappedLine(); } setPreviousRBraceType(TT_EnumRBrace); - if (ManageWhitesmithsBraces) + if (ManageWhitesmithsBraces && CurrentExpandedLines.empty() && + !containsExpansion(*Line)) { Line->MatchingOpeningBlockLineIndex = OpeningLineIndex; + } return true; // There is no addUnwrappedLine() here so that we fall through to parsing a diff --git a/clang/unittests/Format/FormatTestMacroExpansion.cpp b/clang/unittests/Format/FormatTestMacroExpansion.cpp index ca8a5180c32d54..4f35b19b5858f6 100644 --- a/clang/unittests/Format/FormatTestMacroExpansion.cpp +++ b/clang/unittests/Format/FormatTestMacroExpansion.cpp @@ -439,6 +439,119 @@ TEST_F(FormatTestMacroExpansion, TokensAfterMacroCallAreNotPartOfCall) { Style); } +TEST_F(FormatTestMacroExpansion, + SeparateDefinitionBlocksWithMacroExpandedStruct) { + // Reproduces https://llvm.org/PR78263 + FormatStyle Style = getLLVMStyle(); + Style.BreakBeforeBraces = FormatStyle::BS_Custom; + Style.BraceWrapping.AfterStruct = true; + Style.SeparateDefinitionBlocks = FormatStyle::SDS_Always; + Style.Macros.push_back("PACK(x)=x"); + + verifyFormat("static bool foo;\n" + "\n" + "//! \\brief Blah\n" + "PACK(\n" + " struct Foo\n" + " {\n" + " uint16_t a;\n" + " });\n" + "\n" + "PACK(\n" + " struct Bar\n" + " {\n" + " uint32_t a;\n" + " });\n" + "\n" + "PACK(\n" + " struct Baz\n" + " {\n" + " uint16_t a;\n" + " });", + "static bool foo;\n" + "\n" + "//! \\brief Blah\n" + "PACK(struct Foo\n" + "{\n" + " uint16_t a;\n" + "});\n" + "\n" + "PACK(struct Bar\n" + "{\n" + " uint32_t a;\n" + "});\n" + "\n" + "PACK(struct Baz\n" + "{\n" + " uint16_t a;\n" + "});", + Style); +} + +TEST_F(FormatTestMacroExpansion, MacroExpansionWithIfElseBranches) { + // Reproduces https://llvm.org/PR233300 + FormatStyle Style = getLLVMStyle(); + Style.Macros.push_back("M(x)=x"); + + verifyFormat("#if A\n" + "auto l = []\n" + "#else\n" + "if\n" + "#endif\n" + " M((int a) {});", + Style); + + Style.Macros = {"M(a,b,c)=a b(c)"}; + verifyNoCrash("({\n" + "#if A\n" + "{[]\n" + "#else\n" + "if\n" + "#endif\n" + "M(, (int a){int b, ))", + Style); +} + +TEST_F(FormatTestMacroExpansion, RawStringLiteralInMacroArg) { + FormatStyle Style = getGoogleStyle(FormatStyle::LK_Cpp); + Style.Macros.push_back("ASSIGN_OR_RETURN(a, b)=a = (b)"); + + StringRef Code1 = + "void f() {\n" + " ASSIGN_OR_RETURN(FlowConfig flow_config,\n" + " ParseTextProto<FlowConfig>(Substitute(\n" + " R\"pb(\n" + " key: value\n" + " )pb\",\n" + " arg1, arg2, arg3, arg4)));\n" + "}"; + verifyFormat(Code1, Code1, Style); + + StringRef Code2 = + "void f() {\n" + " ASSIGN_OR_RETURN(\n" + " FlowConfig flow_config,\n" + " " + "proto2::contrib::parse_proto::ParseTextProto<FlowConfig>(absl::" + "Substitute(\n" + " R\"pb(\n" + " key: value\n" + " )pb\",\n" + " kPreprocessingFlowConfig, kAudioConfig, kPostConfig)));\n" + "}"; + verifyFormat(Code2, Code2, Style); + + StringRef Code3 = + "void f() {\n" + " ASSIGN_OR_RETURN(\n" + " // comment\n" + " TestCase test_case, ParseTextProto<TestCase>(R\"pb(\n" + " name: \"foo\"\n" + " )pb\"));\n" + "}"; + verifyFormat(Code3, Code3, Style); +} + } // namespace } // namespace test } // namespace format _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
