From efffbee024a8789a0528486d431e27f170b489a7 Mon Sep 17 00:00:00 2001
From: Benjamin Kramer <benny.kra@googlemail.com>
Date: Tue, 9 Sep 2014 13:53:29 +0000
Subject: [PATCH] Tooling: Ignore file names in tooling::deduplicate.

This was horribly broken due to how the sort predicate works. We would
report a conflict for files with a replacement in the same position but
different names if the length differed. Just ignore paths as this is often
what the user wants. Files can occur with different names (due to symlinks
or relative paths) and we don't ever want to do the same edit in one file
twice.

llvm-svn: 217439
---
 clang/include/clang/Tooling/Refactoring.h   |  2 +-
 clang/lib/Tooling/Refactoring.cpp           | 24 ++++++++++++++++-----
 clang/unittests/Tooling/RefactoringTest.cpp |  8 +++++--
 3 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/clang/include/clang/Tooling/Refactoring.h b/clang/include/clang/Tooling/Refactoring.h
index cd2fb9f6c563..3acf16b4247e 100644
--- a/clang/include/clang/Tooling/Refactoring.h
+++ b/clang/include/clang/Tooling/Refactoring.h
@@ -171,7 +171,7 @@ unsigned shiftedCodePosition(const std::vector<Replacement> &Replaces,
                              unsigned Position);
 
 /// \brief Removes duplicate Replacements and reports if Replacements conflict
-/// with one another.
+/// with one another. All Replacements are assumed to be in the same file.
 ///
 /// \post Replaces[i].getOffset() <= Replaces[i+1].getOffset().
 ///
diff --git a/clang/lib/Tooling/Refactoring.cpp b/clang/lib/Tooling/Refactoring.cpp
index b2a02cb0964c..1dadaa7a384c 100644
--- a/clang/lib/Tooling/Refactoring.cpp
+++ b/clang/lib/Tooling/Refactoring.cpp
@@ -238,11 +238,25 @@ void deduplicate(std::vector<Replacement> &Replaces,
   if (Replaces.empty())
     return;
 
-  // Deduplicate
-  std::sort(Replaces.begin(), Replaces.end());
-  std::vector<Replacement>::iterator End =
-      std::unique(Replaces.begin(), Replaces.end());
-  Replaces.erase(End, Replaces.end());
+  auto LessNoPath = [](const Replacement &LHS, const Replacement &RHS) {
+    if (LHS.getOffset() != RHS.getOffset())
+      return LHS.getOffset() < RHS.getOffset();
+    if (LHS.getLength() != RHS.getLength())
+      return LHS.getLength() < RHS.getLength();
+    return LHS.getReplacementText() < RHS.getReplacementText();
+  };
+
+  auto EqualNoPath = [](const Replacement &LHS, const Replacement &RHS) {
+    return LHS.getOffset() == RHS.getOffset() &&
+           LHS.getLength() == RHS.getLength() &&
+           LHS.getReplacementText() == RHS.getReplacementText();
+  };
+
+  // Deduplicate. We don't want to deduplicate based on the path as we assume
+  // that all replacements refer to the same file (or are symlinks).
+  std::sort(Replaces.begin(), Replaces.end(), LessNoPath);
+  Replaces.erase(std::unique(Replaces.begin(), Replaces.end(), EqualNoPath),
+                 Replaces.end());
 
   // Detect conflicts
   Range ConflictRange(Replaces.front().getOffset(),
diff --git a/clang/unittests/Tooling/RefactoringTest.cpp b/clang/unittests/Tooling/RefactoringTest.cpp
index 3e067dd3f99d..73c53aa842a7 100644
--- a/clang/unittests/Tooling/RefactoringTest.cpp
+++ b/clang/unittests/Tooling/RefactoringTest.cpp
@@ -393,6 +393,8 @@ TEST(DeduplicateTest, removesDuplicates) {
   Input.push_back(Replacement("fileA", 50, 0, " foo ")); // Duplicate
   Input.push_back(Replacement("fileA", 51, 3, " bar "));
   Input.push_back(Replacement("fileB", 51, 3, " bar ")); // Filename differs!
+  Input.push_back(Replacement("fileB", 60, 1, " bar "));
+  Input.push_back(Replacement("fileA", 60, 2, " bar "));
   Input.push_back(Replacement("fileA", 51, 3, " moo ")); // Replacement text
                                                          // differs!
 
@@ -403,12 +405,14 @@ TEST(DeduplicateTest, removesDuplicates) {
   Expected.push_back(Replacement("fileA", 50, 0, " foo "));
   Expected.push_back(Replacement("fileA", 51, 3, " bar "));
   Expected.push_back(Replacement("fileA", 51, 3, " moo "));
-  Expected.push_back(Replacement("fileB", 51, 3, " bar "));
+  Expected.push_back(Replacement("fileB", 60, 1, " bar "));
+  Expected.push_back(Replacement("fileA", 60, 2, " bar "));
 
   std::vector<Range> Conflicts; // Ignored for this test
   deduplicate(Input, Conflicts);
 
-  ASSERT_TRUE(Expected == Input);
+  EXPECT_EQ(3U, Conflicts.size());
+  EXPECT_EQ(Expected, Input);
 }
 
 TEST(DeduplicateTest, detectsConflicts) {