From f031aabd0c7104c194f1c956f841fd99b4b1f6e6 Mon Sep 17 00:00:00 2001 From: Barry Revzin Date: Wed, 24 Jun 2026 13:08:54 -0500 Subject: [PATCH 1/3] [clang-format] Fixing erroneous trailing comma --- clang/lib/Tooling/Core/Replacement.cpp | 6 ++++++ clang/unittests/Format/FormatTest.cpp | 13 +++++++++++++ clang/unittests/Tooling/RefactoringTest.cpp | 12 ++++++++++++ 3 files changed, 31 insertions(+) diff --git a/clang/lib/Tooling/Core/Replacement.cpp b/clang/lib/Tooling/Core/Replacement.cpp index 10bdc223e33f2..ac5a2aed8111f 100644 --- a/clang/lib/Tooling/Core/Replacement.cpp +++ b/clang/lib/Tooling/Core/Replacement.cpp @@ -270,6 +270,12 @@ llvm::Error Replacements::add(const Replacement &R) { assert(R.getLength() == 0); // `I` is also an insertion, `R` and `I` conflict. if (I->getLength() == 0) { + // If the two insertions are identical, `R` is redundant; keep the + // existing one rather than concatenating. This happens e.g. when + // clang-format analyzes the same code under several preprocessor + // branches and emits the same insertion in each run. + if (R.getReplacementText() == I->getReplacementText()) + return llvm::Error::success(); // Check if two insertions are order-independent: if inserting them in // either order produces the same text, they are order-independent. if ((R.getReplacementText() + I->getReplacementText()).str() != diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp index c42cc147cf21e..78ddb27e61e95 100644 --- a/clang/unittests/Format/FormatTest.cpp +++ b/clang/unittests/Format/FormatTest.cpp @@ -25210,6 +25210,19 @@ TEST_F(FormatTest, EnumTrailingComma) { " MY_ENUM = 0U\n" "};", Style); + + // Issue https://github.com/llvm/llvm-project/issues/205571 + verifyFormat("#ifdef FOO\n" + "#else\n" + "#endif\n" + "enum {\n" + " E = 1,\n" + "};", + "#ifdef FOO\n" + "#else\n" + "#endif\n" + "enum { E = 1 };", + Style); } TEST_F(FormatTest, BreakAfterAttributes) { diff --git a/clang/unittests/Tooling/RefactoringTest.cpp b/clang/unittests/Tooling/RefactoringTest.cpp index 171dc6de2cae7..7ba989ddb492e 100644 --- a/clang/unittests/Tooling/RefactoringTest.cpp +++ b/clang/unittests/Tooling/RefactoringTest.cpp @@ -416,6 +416,18 @@ TEST_F(ReplacementTest, AddInsertAtOtherInsertWhenOderIndependent) { EXPECT_EQ(Replacement("x.cc", 10, 3, ""), *std::next(Replaces.begin())); } +TEST_F(ReplacementTest, AddIdenticalInsertionsAtSameOffsetDeduplicates) { + Replacements Replaces; + auto Err = Replaces.add(Replacement("x.cc", 10, 0, ",")); + EXPECT_TRUE(!Err); + llvm::consumeError(std::move(Err)); + Err = Replaces.add(Replacement("x.cc", 10, 0, ",")); + EXPECT_TRUE(!Err); + llvm::consumeError(std::move(Err)); + EXPECT_EQ(1u, Replaces.size()); + EXPECT_EQ(Replacement("x.cc", 10, 0, ","), *Replaces.begin()); +} + TEST_F(ReplacementTest, InsertBetweenAdjacentReplacements) { Replacements Replaces; auto Err = Replaces.add(Replacement("x.cc", 10, 5, "a")); From a507b8a17fe82a42b9d4e750bca803d769a1f3f3 Mon Sep 17 00:00:00 2001 From: Barry Revzin Date: Wed, 24 Jun 2026 15:11:31 -0500 Subject: [PATCH 2/3] Skipping duplicates earlier --- clang/lib/Format/TokenAnalyzer.cpp | 10 ++++++++++ clang/lib/Tooling/Core/Replacement.cpp | 6 ------ clang/unittests/Tooling/RefactoringTest.cpp | 12 ------------ 3 files changed, 10 insertions(+), 18 deletions(-) diff --git a/clang/lib/Format/TokenAnalyzer.cpp b/clang/lib/Format/TokenAnalyzer.cpp index 6ba3b05312607..49a48b55598a0 100644 --- a/clang/lib/Format/TokenAnalyzer.cpp +++ b/clang/lib/Format/TokenAnalyzer.cpp @@ -26,6 +26,7 @@ #include "clang/Format/Format.h" #include "llvm/ADT/SmallVector.h" #include "llvm/Support/Debug.h" +#include #define DEBUG_TYPE "format-formatter" @@ -110,6 +111,12 @@ TokenAnalyzer::process(bool SkipAnnotation) { Parser.parse(); assert(UnwrappedLines.back().empty()); unsigned Penalty = 0; + + // The same source region is analyzed once per preprocessor branch, so an + // editing pass (e.g. EnumTrailingCommaEditor) can emit the same replacement + // in more than one run. Ensure that we drop exact duplicates. + std::set SeenReplacements; + for (unsigned Run = 0, RunE = UnwrappedLines.size(); Run + 1 != RunE; ++Run) { const auto &Lines = UnwrappedLines[Run]; LLVM_DEBUG(llvm::dbgs() << "Run " << Run << "...\n"); @@ -136,6 +143,9 @@ TokenAnalyzer::process(bool SkipAnnotation) { Penalty += RunResult.second; for (const auto &R : RunResult.first) { + if (!SeenReplacements.insert(R).second) + continue; + auto Err = Result.add(R); // FIXME: better error handling here. For now, simply return an empty // Replacements to indicate failure. diff --git a/clang/lib/Tooling/Core/Replacement.cpp b/clang/lib/Tooling/Core/Replacement.cpp index ac5a2aed8111f..10bdc223e33f2 100644 --- a/clang/lib/Tooling/Core/Replacement.cpp +++ b/clang/lib/Tooling/Core/Replacement.cpp @@ -270,12 +270,6 @@ llvm::Error Replacements::add(const Replacement &R) { assert(R.getLength() == 0); // `I` is also an insertion, `R` and `I` conflict. if (I->getLength() == 0) { - // If the two insertions are identical, `R` is redundant; keep the - // existing one rather than concatenating. This happens e.g. when - // clang-format analyzes the same code under several preprocessor - // branches and emits the same insertion in each run. - if (R.getReplacementText() == I->getReplacementText()) - return llvm::Error::success(); // Check if two insertions are order-independent: if inserting them in // either order produces the same text, they are order-independent. if ((R.getReplacementText() + I->getReplacementText()).str() != diff --git a/clang/unittests/Tooling/RefactoringTest.cpp b/clang/unittests/Tooling/RefactoringTest.cpp index 7ba989ddb492e..171dc6de2cae7 100644 --- a/clang/unittests/Tooling/RefactoringTest.cpp +++ b/clang/unittests/Tooling/RefactoringTest.cpp @@ -416,18 +416,6 @@ TEST_F(ReplacementTest, AddInsertAtOtherInsertWhenOderIndependent) { EXPECT_EQ(Replacement("x.cc", 10, 3, ""), *std::next(Replaces.begin())); } -TEST_F(ReplacementTest, AddIdenticalInsertionsAtSameOffsetDeduplicates) { - Replacements Replaces; - auto Err = Replaces.add(Replacement("x.cc", 10, 0, ",")); - EXPECT_TRUE(!Err); - llvm::consumeError(std::move(Err)); - Err = Replaces.add(Replacement("x.cc", 10, 0, ",")); - EXPECT_TRUE(!Err); - llvm::consumeError(std::move(Err)); - EXPECT_EQ(1u, Replaces.size()); - EXPECT_EQ(Replacement("x.cc", 10, 0, ","), *Replaces.begin()); -} - TEST_F(ReplacementTest, InsertBetweenAdjacentReplacements) { Replacements Replaces; auto Err = Replaces.add(Replacement("x.cc", 10, 5, "a")); From 2693efef589b7432b422580077bd73a74889c83c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Sch=C3=A4pers?= Date: Thu, 25 Jun 2026 23:19:55 +0200 Subject: [PATCH 3/3] Properly fix it --- clang/lib/Format/Format.cpp | 9 ++++++--- clang/lib/Format/FormatToken.h | 9 ++++++--- clang/lib/Format/TokenAnalyzer.cpp | 10 ---------- 3 files changed, 12 insertions(+), 16 deletions(-) diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp index 51a976783b589..ecfe5d2ce60d0 100644 --- a/clang/lib/Format/Format.cpp +++ b/clang/lib/Format/Format.cpp @@ -2824,12 +2824,12 @@ class EnumTrailingCommaEditor : public TokenAnalyzer { void editEnumTrailingComma(SmallVectorImpl &Lines, tooling::Replacements &Result) { bool InEnumBraces = false; - const FormatToken *BeforeRBrace = nullptr; + FormatToken *BeforeRBrace = nullptr; const auto &SourceMgr = Env.getSourceManager(); for (auto *Line : Lines) { if (!Line->Children.empty()) editEnumTrailingComma(Line->Children, Result); - for (const auto *Token = Line->First; Token && !Token->Finalized; + for (auto *Token = Line->First; Token && !Token->Finalized; Token = Token->Next) { if (Token->isNot(TT_EnumRBrace)) { if (Token->is(TT_EnumLBrace)) @@ -2839,8 +2839,10 @@ class EnumTrailingCommaEditor : public TokenAnalyzer { continue; } InEnumBraces = false; - if (!BeforeRBrace) // Empty braces or Line not affected. + if (!BeforeRBrace || BeforeRBrace->HasEnumTrailingCommaHandled) { + // Empty braces, or Line not affected, or already handled. continue; + } if (BeforeRBrace->is(tok::comma)) { if (Style.EnumTrailingComma == FormatStyle::ETC_Remove) replaceToken(*BeforeRBrace, BeforeRBrace->Next, SourceMgr, Result); @@ -2848,6 +2850,7 @@ class EnumTrailingCommaEditor : public TokenAnalyzer { cantFail(Result.add(tooling::Replacement( SourceMgr, BeforeRBrace->Tok.getEndLoc(), 0, ","))); } + BeforeRBrace->HasEnumTrailingCommaHandled = true; BeforeRBrace = nullptr; } } diff --git a/clang/lib/Format/FormatToken.h b/clang/lib/Format/FormatToken.h index 556bb0f3dd0af..b6dfa440b45a1 100644 --- a/clang/lib/Format/FormatToken.h +++ b/clang/lib/Format/FormatToken.h @@ -330,9 +330,9 @@ struct FormatToken { EndsBinaryExpression(false), PartOfMultiVariableDeclStmt(false), ContinuesLineCommentSection(false), Finalized(false), ClosesRequiresClause(false), EndsCppAttributeGroup(false), - BlockKind(BK_Unknown), Decision(FD_Unformatted), - PackingKind(PPK_Inconclusive), TypeIsFinalized(false), - Type(TT_Unknown) {} + HasEnumTrailingCommaHandled(false), BlockKind(BK_Unknown), + Decision(FD_Unformatted), PackingKind(PPK_Inconclusive), + TypeIsFinalized(false), Type(TT_Unknown) {} /// The \c Token. Token Tok; @@ -408,6 +408,9 @@ struct FormatToken { /// \c true if this token ends a group of C++ attributes. unsigned EndsCppAttributeGroup : 1; + /// \c true if a comma has been inserted or removed after the token. + unsigned HasEnumTrailingCommaHandled : 1; + private: /// Contains the kind of block if this token is a brace. unsigned BlockKind : 2; diff --git a/clang/lib/Format/TokenAnalyzer.cpp b/clang/lib/Format/TokenAnalyzer.cpp index 49a48b55598a0..6ba3b05312607 100644 --- a/clang/lib/Format/TokenAnalyzer.cpp +++ b/clang/lib/Format/TokenAnalyzer.cpp @@ -26,7 +26,6 @@ #include "clang/Format/Format.h" #include "llvm/ADT/SmallVector.h" #include "llvm/Support/Debug.h" -#include #define DEBUG_TYPE "format-formatter" @@ -111,12 +110,6 @@ TokenAnalyzer::process(bool SkipAnnotation) { Parser.parse(); assert(UnwrappedLines.back().empty()); unsigned Penalty = 0; - - // The same source region is analyzed once per preprocessor branch, so an - // editing pass (e.g. EnumTrailingCommaEditor) can emit the same replacement - // in more than one run. Ensure that we drop exact duplicates. - std::set SeenReplacements; - for (unsigned Run = 0, RunE = UnwrappedLines.size(); Run + 1 != RunE; ++Run) { const auto &Lines = UnwrappedLines[Run]; LLVM_DEBUG(llvm::dbgs() << "Run " << Run << "...\n"); @@ -143,9 +136,6 @@ TokenAnalyzer::process(bool SkipAnnotation) { Penalty += RunResult.second; for (const auto &R : RunResult.first) { - if (!SeenReplacements.insert(R).second) - continue; - auto Err = Result.add(R); // FIXME: better error handling here. For now, simply return an empty // Replacements to indicate failure.