From 62d3f3f44d58b49fe769fc36ae0d2eeb386c36a0 Mon Sep 17 00:00:00 2001 From: Benjamin Kramer 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. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@217439 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Tooling/Refactoring.h | 2 +- lib/Tooling/Refactoring.cpp | 24 +++++++++++++++++++----- unittests/Tooling/RefactoringTest.cpp | 8 ++++++-- 3 files changed, 26 insertions(+), 8 deletions(-) diff --git a/include/clang/Tooling/Refactoring.h b/include/clang/Tooling/Refactoring.h index cd2fb9f6c5..3acf16b424 100644 --- a/include/clang/Tooling/Refactoring.h +++ b/include/clang/Tooling/Refactoring.h @@ -171,7 +171,7 @@ unsigned shiftedCodePosition(const std::vector &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/lib/Tooling/Refactoring.cpp b/lib/Tooling/Refactoring.cpp index b2a02cb096..1dadaa7a38 100644 --- a/lib/Tooling/Refactoring.cpp +++ b/lib/Tooling/Refactoring.cpp @@ -238,11 +238,25 @@ void deduplicate(std::vector &Replaces, if (Replaces.empty()) return; - // Deduplicate - std::sort(Replaces.begin(), Replaces.end()); - std::vector::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/unittests/Tooling/RefactoringTest.cpp b/unittests/Tooling/RefactoringTest.cpp index 3e067dd3f9..73c53aa842 100644 --- a/unittests/Tooling/RefactoringTest.cpp +++ b/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 Conflicts; // Ignored for this test deduplicate(Input, Conflicts); - ASSERT_TRUE(Expected == Input); + EXPECT_EQ(3U, Conflicts.size()); + EXPECT_EQ(Expected, Input); } TEST(DeduplicateTest, detectsConflicts) { -- 2.40.0