Deduplicate sets of replacements by file names.

Summary:
If there are multiple <File, Replacements> pairs with the same file
path after removing dots, we only keep one pair (with path after dots being
removed) and discard the rest.

Reviewers: djasper

Subscribers: klimek, hokein, bkramer, cfe-commits

Differential Revision: https://reviews.llvm.org/D25565

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@284219 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/Tooling/Core/Replacement.h b/include/clang/Tooling/Core/Replacement.h
index e57afc7f851482b6af177571668c89a63c8dab1a..2ac180b1ce92582567fdaa84e46e61e870f23b89 100644 (file)
--- a/include/clang/Tooling/Core/Replacement.h
+++ b/include/clang/Tooling/Core/Replacement.h
@@ -230,8 +230,6 @@ private:
   Replacements(const_iterator Begin, const_iterator End)
       : Replaces(Begin, End) {}
 
-  Replacements mergeReplacements(const ReplacementsImpl &Second) const;
-
   // Returns `R` with new range that refers to code after `Replaces` being
   // applied.
   Replacement getReplacementInChangedCode(const Replacement &R) const;
@@ -294,10 +292,11 @@ std::vector<Range>
 calculateRangesAfterReplacements(const Replacements &Replaces,
                                  const std::vector<Range> &Ranges);
 
-/// \brief Groups a random set of replacements by file path. Replacements
-/// related to the same file entry are put into the same vector.
-std::map<std::string, Replacements>
-groupReplacementsByFile(const Replacements &Replaces);
+/// \brief If there are multiple <File, Replacements> pairs with the same file
+/// path after removing dots, we only keep one pair (with path after dots being
+/// removed) and discard the rest.
+std::map<std::string, Replacements> groupReplacementsByFile(
+    const std::map<std::string, Replacements> &FileToReplaces);
 
 template <typename Node>
 Replacement::Replacement(const SourceManager &Sources,

diff --git a/include/clang/Tooling/Refactoring.h b/include/clang/Tooling/Refactoring.h
index 7a5f9dd45828224c18c2e1e70a8cec767971e40a..bc95c3b09ec2d0608469a50748353aa5acc38f2a 100644 (file)
--- a/include/clang/Tooling/Refactoring.h
+++ b/include/clang/Tooling/Refactoring.h
@@ -55,6 +55,9 @@ public:
 
   /// \brief Apply all stored replacements to the given Rewriter.
   ///
+  /// FileToReplaces will be deduplicated with `groupReplacementsByFile` before
+  /// application.
+  ///
   /// Replacement applications happen independently of the success of other
   /// applications.
   ///
@@ -75,6 +78,9 @@ private:
 ///
 /// \pre Replacements must be conflict-free.
 ///
+/// FileToReplaces will be deduplicated with `groupReplacementsByFile` before
+/// application.
+///
 /// Replacement applications happen independently of the success of other
 /// applications.
 ///

diff --git a/lib/Tooling/Core/Replacement.cpp b/lib/Tooling/Core/Replacement.cpp
index bdca474015e0557cd567d2251b74aab76c1a42e4..088b320916a819441a94f601c66bfa491b6de37b 100644 (file)
--- a/lib/Tooling/Core/Replacement.cpp
+++ b/lib/Tooling/Core/Replacement.cpp
@@ -21,6 +21,7 @@
 #include "clang/Lex/Lexer.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/raw_os_ostream.h"
 
 namespace clang {
@@ -564,13 +565,15 @@ llvm::Expected<std::string> applyAllReplacements(StringRef Code,
   return Result;
 }
 
-std::map<std::string, Replacements>
-groupReplacementsByFile(const Replacements &Replaces) {
-  std::map<std::string, Replacements> FileToReplaces;
-  for (const auto &Replace : Replaces)
-    // We can ignore the Error here since \p Replaces is already conflict-free.
-    FileToReplaces[Replace.getFilePath()].add(Replace);
-  return FileToReplaces;
+std::map<std::string, Replacements> groupReplacementsByFile(
+    const std::map<std::string, Replacements> &FileToReplaces) {
+  std::map<std::string, Replacements> Result;
+  for (const auto &Entry : FileToReplaces) {
+    llvm::SmallString<256> CleanPath(Entry.first.data());
+    llvm::sys::path::remove_dots(CleanPath, /*remove_dot_dot=*/true);
+    Result[CleanPath.str()] = std::move(Entry.second);
+  }
+  return Result;
 }
 
 } // end namespace tooling

diff --git a/lib/Tooling/Refactoring.cpp b/lib/Tooling/Refactoring.cpp
index 5565b5499c7877697ac5c90455d6ba893f4b02d6..241557d8c49193c4ccc65612d045a86f329ddb5f 100644 (file)
--- a/lib/Tooling/Refactoring.cpp
+++ b/lib/Tooling/Refactoring.cpp
@@ -57,7 +57,7 @@ int RefactoringTool::runAndSave(FrontendActionFactory *ActionFactory) {
 
 bool RefactoringTool::applyAllReplacements(Rewriter &Rewrite) {
   bool Result = true;
-  for (const auto &Entry : FileToReplaces)
+  for (const auto &Entry : groupReplacementsByFile(FileToReplaces))
     Result = tooling::applyAllReplacements(Entry.second, Rewrite) && Result;
   return Result;
 }
@@ -73,7 +73,7 @@ bool formatAndApplyAllReplacements(
   FileManager &Files = SM.getFileManager();
 
   bool Result = true;
-  for (const auto &FileAndReplaces : FileToReplaces) {
+  for (const auto &FileAndReplaces : groupReplacementsByFile(FileToReplaces)) {
     const std::string &FilePath = FileAndReplaces.first;
     auto &CurReplaces = FileAndReplaces.second;
 

diff --git a/unittests/Tooling/RefactoringTest.cpp b/unittests/Tooling/RefactoringTest.cpp
index 0a70a112aa8daeb48edd0896c8d7836183eb9678..bf50b7d94d422db895c8c3d5012c38639aa09c9b 100644 (file)
--- a/unittests/Tooling/RefactoringTest.cpp
+++ b/unittests/Tooling/RefactoringTest.cpp
@@ -972,5 +972,23 @@ TEST_F(MergeReplacementsTest, OverlappingRanges) {
       toReplacements({{"", 0, 3, "cc"}, {"", 3, 3, "dd"}}));
 }
 
+TEST(DeduplicateByFileTest, LeaveLeadingDotDot) {
+  std::map<std::string, Replacements> FileToReplaces;
+  FileToReplaces["../../a/b/.././c.h"] = Replacements();
+  FileToReplaces["../../a/c.h"] = Replacements();
+  FileToReplaces = groupReplacementsByFile(FileToReplaces);
+  EXPECT_EQ(1u, FileToReplaces.size());
+  EXPECT_EQ("../../a/c.h", FileToReplaces.begin()->first);
+}
+
+TEST(DeduplicateByFileTest, RemoveDotSlash) {
+  std::map<std::string, Replacements> FileToReplaces;
+  FileToReplaces["./a/b/.././c.h"] = Replacements();
+  FileToReplaces["a/c.h"] = Replacements();
+  FileToReplaces = groupReplacementsByFile(FileToReplaces);
+  EXPECT_EQ(1u, FileToReplaces.size());
+  EXPECT_EQ("a/c.h", FileToReplaces.begin()->first);
+}
+
 } // end namespace tooling
 } // end namespace clang