[clang-format] Fixing erroneous trailing comma

[brevzin]

Fixes #205571

[HazardyKnusperkeks]

This fixes the symptom, while the root cause is kept. I'm currently not that fit in the preprocessor branches stuff, but it would be nice if we can fix it there. It it outside of the branches and an unwrapped line on itself.

[github-actions]

🐧 Linux x64 Test Results

• 118522 tests passed
• 4905 tests skipped

✅ The build succeeded and all tests passed.

[github-actions]

🪟 Windows x64 Test Results

• 55769 tests passed
• 2702 tests skipped

✅ The build succeeded and all tests passed.

[llvmorg-github-actions]

@llvm/pr-subscribers-clang-format

Author: Barry Revzin (brevzin)

Changes

Fixes #205571

---

Full diff: https://github.com/llvm/llvm-project/pull/205631.diff

3 Files Affected:

• (modified) clang/lib/Tooling/Core/Replacement.cpp (+6)
• (modified) clang/unittests/Format/FormatTest.cpp (+13)
• (modified) clang/unittests/Tooling/RefactoringTest.cpp (+12)

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"));

[HazardyKnusperkeks]

You would've still only handled the symptom, now with a dynamic allocation hit for everyone. Please take a look at my fix.

[brevzin]

You would've still only handled the symptom, now with a dynamic allocation hit for everyone. Please take a look at my fix.

Makes a lot of sense to me, thanks!

[llvm-ci]

LLVM Buildbot has detected a new failure on builder clang-aarch64-quick running on linaro-clang-aarch64-quick while building clang at step 5 "ninja check 1".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/36414

Here is the relevant piece of the build log for the reference

Step 5 (ninja check 1) failure: 1200 seconds without output running [b'ninja', b'check-all'], attempting to kill
...
[161/165] Linking CXX executable unittests/ObjectYAML/ObjectYAMLTests
[162/165] Building CXX object tools/clang/unittests/Format/CMakeFiles/FormatTests.dir/TokenAnnotatorTest.cpp.o
[163/165] Building CXX object tools/clang/unittests/Format/CMakeFiles/FormatTests.dir/FormatTest.cpp.o
[164/165] Linking CXX executable tools/clang/unittests/Format/FormatTests
[164/165] Running all regression tests
llvm-lit: /home/tcwg-buildbot/worker/clang-aarch64-quick/llvm/llvm/utils/lit/lit/llvm/config.py:569: note: using split-file: /home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/bin/split-file
llvm-lit: /home/tcwg-buildbot/worker/clang-aarch64-quick/llvm/llvm/utils/lit/lit/llvm/config.py:569: note: using yaml2obj: /home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/bin/yaml2obj
llvm-lit: /home/tcwg-buildbot/worker/clang-aarch64-quick/llvm/llvm/utils/lit/lit/llvm/config.py:569: note: using llvm-objcopy: /home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/bin/llvm-objcopy
llvm-lit: /home/tcwg-buildbot/worker/clang-aarch64-quick/llvm/llvm/utils/lit/lit/llvm/config.py:569: note: using clang: /home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/bin/clang
-- Testing: 96105 of 96108 tests, 256 workers --
command timed out: 1200 seconds without output running [b'ninja', b'check-all'], attempting to kill
process killed by signal 9
program finished with exit code -1
elapsedTime=1264.412056