Deduplicate sets of replacements by file names.

[ioeric]

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.

[ioeric]

• Updated comments for RefactoringTool.

[ioeric]

include/clang/Tooling/Core/Replacement.h

233 ↗

(On Diff #74531)

This is dead code.

[djasper]

I'd not rename the function. Otherwise looks good.

[ioeric]

• Change name to groupReplacementsByFile

[ioeric]

• Merge branch 'master' of http://llvm.org/git/clang into arcpatch-D25565
• Forgot to update names in tests...

cfe/trunk/include/clang/Tooling/Core/Replacement.h

bool operator==(const Replacements &RHS) const {
     return Replaces == RHS.Replaces;
   }
 
 
 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;
 
   // Returns a set of replacements that is equivalent to the current
   // replacements by merging all adjacent replacements. Two sets of replacements
   // are considered equivalent if they have the same effect when they are
   // applied.

 /// \pre Replacements must be for the same file.
 ///
 /// \return The new ranges after \p Replaces are applied. The new ranges will be
 /// sorted and non-overlapping.
 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,
                          const Node &NodeToReplace, StringRef ReplacementText,
                          const LangOptions &LangOpts) {
   const CharSourceRange Range =
       CharSourceRange::getTokenRange(NodeToReplace->getSourceRange());
   setFromSourceRange(Sources, Range, ReplacementText, LangOpts);
 }
 
 } // end namespace tooling
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLING_CORE_REPLACEMENT_H

cfe/trunk/include/clang/Tooling/Refactoring.h

public:
   /// \brief Call run(), apply all generated replacements, and immediately save
   /// the results to disk.
   ///
   /// \returns 0 upon success. Non-zero upon failure.
   int runAndSave(FrontendActionFactory *ActionFactory);
 
   /// \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.
   ///
   /// \returns true if all replacements apply. false otherwise.
   bool applyAllReplacements(Rewriter &Rewrite);
 
 private:
   /// \brief Write all refactored files to disk.
   int saveRewrittenFiles(Rewriter &Rewrite);
 
 private:
   std::map<std::string, Replacements> FileToReplaces;
 };
 
 /// \brief Groups \p Replaces by the file path and applies each group of
 /// Replacements on the related file in \p Rewriter. In addition to applying
 /// given Replacements, this function also formats the changed code.
 ///
 /// \pre Replacements must be conflict-free.
 ///
+/// FileToReplaces will be deduplicated with `groupReplacementsByFile` before
+/// application.
+///
 /// Replacement applications happen independently of the success of other
 /// applications.
 ///
 /// \param[in] FileToReplaces Replacements (grouped by files) to apply.
 /// \param[in] Rewrite The `Rewritter` to apply replacements on.
 /// \param[in] Style The style name used for reformatting. See ```getStyle``` in
 /// "include/clang/Format/Format.h" for all possible style forms.
 ///

cfe/trunk/lib/Tooling/Core/Replacement.cpp

 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticIDs.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
 #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 {
 namespace tooling {
 
 static const char * const InvalidLocation = "";
 
 Replacement::Replacement()
llvm::Expected<std::string> applyAllReplacements(StringRef Code,
   }
   std::string Result;
   llvm::raw_string_ostream OS(Result);
   Rewrite.getEditBuffer(ID).write(OS);
   OS.flush();
   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
 } // end namespace clang

cfe/trunk/lib/Tooling/Refactoring.cpp

if (!applyAllReplacements(Rewrite)) {
     llvm::errs() << "Skipped some replacements.\n";
   }
 
   return saveRewrittenFiles(Rewrite);
 }
 
 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;
 }
 
 int RefactoringTool::saveRewrittenFiles(Rewriter &Rewrite) {
   return Rewrite.overwriteChangedFiles() ? 1 : 0;
 }
 
 bool formatAndApplyAllReplacements(
     const std::map<std::string, Replacements> &FileToReplaces, Rewriter &Rewrite,
     StringRef Style) {
   SourceManager &SM = Rewrite.getSourceMgr();
   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;
 
     const FileEntry *Entry = Files.getFile(FilePath);
     FileID ID = SM.getOrCreateFileID(Entry, SrcMgr::C_User);
     StringRef Code = SM.getBufferData(ID);
 
     format::FormatStyle CurStyle = format::getStyle(Style, FilePath, "LLVM");

cfe/trunk/unittests/Tooling/RefactoringTest.cpp

mergeAndTestRewrite(
       toReplacements({{"", 0, 1, ""}, {"", 2, 0, "bb"}, {"", 3, 0, "b"}}),
       toReplacements({{"", 0, 2, "cc"}, {"", 2, 3, "dd"}}));
   mergeAndTestRewrite(
       "aa", "babbab", "ccdd",
       toReplacements({{"", 0, 0, "b"}, {"", 1, 0, "bb"}, {"", 2, 0, "b"}}),
       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