r284219 - Deduplicate sets of replacements by file names.

[Eric Liu via cfe-commits]

Author: ioeric
Date: Fri Oct 14 04:32:06 2016
New Revision: 284219

URL: http://llvm.org/viewvc/llvm-project?rev=284219&view=rev
Log:
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

Modified:
    cfe/trunk/include/clang/Tooling/Core/Replacement.h
    cfe/trunk/include/clang/Tooling/Refactoring.h
    cfe/trunk/lib/Tooling/Core/Replacement.cpp
    cfe/trunk/lib/Tooling/Refactoring.cpp
    cfe/trunk/unittests/Tooling/RefactoringTest.cpp

Modified: cfe/trunk/include/clang/Tooling/Core/Replacement.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/Core/Replacement.h?rev=284219&r1=284218&r2=284219&view=diff
==============================================================================
--- cfe/trunk/include/clang/Tooling/Core/Replacement.h (original)
+++ cfe/trunk/include/clang/Tooling/Core/Replacement.h Fri Oct 14 04:32:06 2016
@@ -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,

Modified: cfe/trunk/include/clang/Tooling/Refactoring.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/Refactoring.h?rev=284219&r1=284218&r2=284219&view=diff
==============================================================================
--- cfe/trunk/include/clang/Tooling/Refactoring.h (original)
+++ cfe/trunk/include/clang/Tooling/Refactoring.h Fri Oct 14 04:32:06 2016
@@ -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.
 ///

Modified: cfe/trunk/lib/Tooling/Core/Replacement.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/Core/Replacement.cpp?rev=284219&r1=284218&r2=284219&view=diff
==============================================================================
--- cfe/trunk/lib/Tooling/Core/Replacement.cpp (original)
+++ cfe/trunk/lib/Tooling/Core/Replacement.cpp Fri Oct 14 04:32:06 2016
@@ -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> applyAllRepl
   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

Modified: cfe/trunk/lib/Tooling/Refactoring.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/Refactoring.cpp?rev=284219&r1=284218&r2=284219&view=diff
==============================================================================
--- cfe/trunk/lib/Tooling/Refactoring.cpp (original)
+++ cfe/trunk/lib/Tooling/Refactoring.cpp Fri Oct 14 04:32:06 2016
@@ -57,7 +57,7 @@ int RefactoringTool::runAndSave(Frontend
 
 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;
 

Modified: cfe/trunk/unittests/Tooling/RefactoringTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/RefactoringTest.cpp?rev=284219&r1=284218&r2=284219&view=diff
==============================================================================
--- cfe/trunk/unittests/Tooling/RefactoringTest.cpp (original)
+++ cfe/trunk/unittests/Tooling/RefactoringTest.cpp Fri Oct 14 04:32:06 2016
@@ -972,5 +972,23 @@ TEST_F(MergeReplacementsTest, Overlappin
       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