[clang-apply-replacements] Convert tooling::Replacements to tooling::AtomicChange...
authorMalcolm Parsons <malcolm.parsons@gmail.com>
Wed, 11 Apr 2018 14:39:17 +0000 (14:39 +0000)
committerMalcolm Parsons <malcolm.parsons@gmail.com>
Wed, 11 Apr 2018 14:39:17 +0000 (14:39 +0000)
Summary:
By converting Replacements by AtomicChange, clang-apply-replacements is able like clang-tidy to automatically cleanup and format changes.
This should permits to close this ticket: https://bugs.llvm.org/show_bug.cgi?id=35051 and attempt to follow hints from https://reviews.llvm.org/D43500 comments.

Reviewers: klimek, ioeric

Reviewed By: ioeric

Subscribers: malcolm.parsons, mgorny, cfe-commits

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

Patch by Jeremy Demeule.

llvm-svn: 329813

17 files changed:
clang-tools-extra/clang-apply-replacements/include/clang-apply-replacements/Tooling/ApplyReplacements.h
clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
clang-tools-extra/clang-apply-replacements/tool/CMakeLists.txt
clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
clang-tools-extra/test/clang-apply-replacements/Inputs/basic/file2.yaml
clang-tools-extra/test/clang-apply-replacements/Inputs/conflict/expected.txt
clang-tools-extra/test/clang-apply-replacements/Inputs/identical/file1.yaml [new file with mode: 0644]
clang-tools-extra/test/clang-apply-replacements/Inputs/identical/identical.cpp [new file with mode: 0644]
clang-tools-extra/test/clang-apply-replacements/Inputs/order-dependent/expected.txt [new file with mode: 0644]
clang-tools-extra/test/clang-apply-replacements/Inputs/order-dependent/file1.yaml [new file with mode: 0644]
clang-tools-extra/test/clang-apply-replacements/Inputs/order-dependent/file2.yaml [new file with mode: 0644]
clang-tools-extra/test/clang-apply-replacements/Inputs/order-dependent/order-dependent.cpp [new file with mode: 0644]
clang-tools-extra/test/clang-apply-replacements/identical.cpp [new file with mode: 0644]
clang-tools-extra/test/clang-apply-replacements/order-dependent.cpp [new file with mode: 0644]
clang-tools-extra/unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
clang-tools-extra/unittests/clang-apply-replacements/CMakeLists.txt
clang-tools-extra/unittests/clang-apply-replacements/ReformattingTest.cpp [deleted file]

index 5e0ff48..da9ae0c 100644 (file)
@@ -18,6 +18,7 @@
 
 #include "clang/Tooling/Core/Diagnostic.h"
 #include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Refactoring/AtomicChange.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include <string>
@@ -29,15 +30,8 @@ namespace clang {
 class DiagnosticsEngine;
 class Rewriter;
 
-namespace format {
-struct FormatStyle;
-} // end namespace format
-
 namespace replace {
 
-/// \brief Collection of source ranges.
-typedef std::vector<clang::tooling::Range> RangeVector;
-
 /// \brief Collection of TranslationUnitReplacements.
 typedef std::vector<clang::tooling::TranslationUnitReplacements> TUReplacements;
 
@@ -47,10 +41,10 @@ typedef std::vector<std::string> TUReplacementFiles;
 /// \brief Collection of TranslationUniDiagnostics.
 typedef std::vector<clang::tooling::TranslationUnitDiagnostics> TUDiagnostics;
 
-/// \brief Map mapping file name to Replacements targeting that file.
+/// \brief Map mapping file name to a set of AtomicChange targeting that file.
 typedef llvm::DenseMap<const clang::FileEntry *,
-                       std::vector<clang::tooling::Replacement>>
-    FileToReplacementsMap;
+                       std::vector<tooling::AtomicChange>>
+    FileToChangesMap;
 
 /// \brief Recursively descends through a directory structure rooted at \p
 /// Directory and attempts to deserialize *.yaml files as
@@ -77,65 +71,39 @@ std::error_code collectReplacementsFromDirectory(
     const llvm::StringRef Directory, TUDiagnostics &TUs,
     TUReplacementFiles &TUFiles, clang::DiagnosticsEngine &Diagnostics);
 
-/// \brief Deduplicate, check for conflicts, and apply all Replacements stored
-/// in \c TUs. If conflicts occur, no Replacements are applied.
+/// \brief Deduplicate, check for conflicts, and extract all Replacements stored
+/// in \c TUs. Conflicting replacements are skipped.
 ///
-/// \post For all (key,value) in GroupedReplacements, value[i].getOffset() <=
+/// \post For all (key,value) in FileChanges, value[i].getOffset() <=
 /// value[i+1].getOffset().
 ///
 /// \param[in] TUs Collection of TranslationUnitReplacements or
-/// TranslationUnitDiagnostics to merge,
-/// deduplicate, and test for conflicts.
-/// \param[out] GroupedReplacements Container grouping all Replacements by the
-/// file they target.
+/// TranslationUnitDiagnostics to merge, deduplicate, and test for conflicts.
+/// \param[out] FileChanges Container grouping all changes by the
+/// file they target. Only non conflicting replacements are kept into
+/// FileChanges.
 /// \param[in] SM SourceManager required for conflict reporting.
 ///
 /// \returns \parblock
-///          \li true If all changes were applied successfully.
+///          \li true If all changes were converted successfully.
 ///          \li false If there were conflicts.
-bool mergeAndDeduplicate(const TUReplacements &TUs,
-                         FileToReplacementsMap &GroupedReplacements,
-                         clang::SourceManager &SM);
-
-bool mergeAndDeduplicate(const TUDiagnostics &TUs,
-                         FileToReplacementsMap &GroupedReplacements,
+bool mergeAndDeduplicate(const TUReplacements &TUs, const TUDiagnostics &TUDs,
+                         FileToChangesMap &FileChanges,
                          clang::SourceManager &SM);
 
-// FIXME: Remove this function after changing clang-apply-replacements to use
-// Replacements class.
-bool applyAllReplacements(const std::vector<tooling::Replacement> &Replaces,
-                          Rewriter &Rewrite);
-
-/// \brief Apply all replacements in \c GroupedReplacements.
-///
-/// \param[in] GroupedReplacements Deduplicated and conflict free Replacements
-/// to apply.
-/// \param[out] Rewrites The results of applying replacements will be applied
-/// to this Rewriter.
+/// \brief Apply \c AtomicChange on File and rewrite it.
 ///
-/// \returns \parblock
-///          \li true If all changes were applied successfully.
-///          \li false If a replacement failed to apply.
-bool applyReplacements(const FileToReplacementsMap &GroupedReplacements,
-                       clang::Rewriter &Rewrites);
-
-/// \brief Given a collection of Replacements for a single file, produces a list
-/// of source ranges that enclose those Replacements.
-///
-/// \pre Replacements[i].getOffset() <= Replacements[i+1].getOffset().
-///
-/// \param[in] Replacements Replacements from a single file.
-///
-/// \returns Collection of source ranges that enclose all given Replacements.
-/// One range is created for each replacement.
-RangeVector calculateChangedRanges(
-    const std::vector<clang::tooling::Replacement> &Replacements);
-
-/// \brief Write the contents of \c FileContents to disk. Keys of the map are
-/// filenames and values are the new contents for those files.
+/// \param[in] File Path of the file where to apply AtomicChange.
+/// \param[in] Changes to apply.
+/// \param[in] Spec For code cleanup and formatting.
+/// \param[in] Diagnostics DiagnosticsEngine used for error output.
 ///
-/// \param[in] Rewrites Rewriter containing written files to write to disk.
-bool writeFiles(const clang::Rewriter &Rewrites);
+/// \returns The changed code if all changes are applied successfully;
+/// otherwise, an llvm::Error carrying llvm::StringError or an error_code.
+llvm::Expected<std::string>
+applyChanges(StringRef File, const std::vector<tooling::AtomicChange> &Changes,
+             const tooling::ApplyChangesSpec &Spec,
+             DiagnosticsEngine &Diagnostics);
 
 /// \brief Delete the replacement files.
 ///
index 56482dc..1970d05 100644 (file)
@@ -80,10 +80,9 @@ std::error_code collectReplacementsFromDirectory(
   return ErrorCode;
 }
 
-std::error_code
-collectReplacementsFromDirectory(const llvm::StringRef Directory,
-                                 TUDiagnostics &TUs, TUReplacementFiles &TUFiles,
-                                 clang::DiagnosticsEngine &Diagnostics) {
+std::error_code collectReplacementsFromDirectory(
+    const llvm::StringRef Directory, TUDiagnostics &TUs,
+    TUReplacementFiles &TUFiles, clang::DiagnosticsEngine &Diagnostics) {
   using namespace llvm::sys::fs;
   using namespace llvm::sys::path;
 
@@ -125,259 +124,109 @@ collectReplacementsFromDirectory(const llvm::StringRef Directory,
   return ErrorCode;
 }
 
-/// \brief Dumps information for a sequence of conflicting Replacements.
+/// \brief Extract replacements from collected TranslationUnitReplacements and
+/// TranslationUnitDiagnostics and group them per file.
 ///
-/// \param[in] File FileEntry for the file the conflicting Replacements are
-/// for.
-/// \param[in] ConflictingReplacements List of conflicting Replacements.
-/// \param[in] SM SourceManager used for reporting.
-static void reportConflict(
-    const FileEntry *File,
-    const llvm::ArrayRef<clang::tooling::Replacement> ConflictingReplacements,
-    SourceManager &SM) {
-  FileID FID = SM.translateFile(File);
-  if (FID.isInvalid())
-    FID = SM.createFileID(File, SourceLocation(), SrcMgr::C_User);
-
-  // FIXME: Output something a little more user-friendly (e.g. unified diff?)
-  errs() << "The following changes conflict:\n";
-  for (const tooling::Replacement &R : ConflictingReplacements) {
-    if (R.getLength() == 0) {
-      errs() << "  Insert at " << SM.getLineNumber(FID, R.getOffset()) << ":"
-             << SM.getColumnNumber(FID, R.getOffset()) << " "
-             << R.getReplacementText() << "\n";
-    } else {
-      if (R.getReplacementText().empty())
-        errs() << "  Remove ";
-      else
-        errs() << "  Replace ";
-
-      errs() << SM.getLineNumber(FID, R.getOffset()) << ":"
-             << SM.getColumnNumber(FID, R.getOffset()) << "-"
-             << SM.getLineNumber(FID, R.getOffset() + R.getLength() - 1) << ":"
-             << SM.getColumnNumber(FID, R.getOffset() + R.getLength() - 1);
-
-      if (R.getReplacementText().empty())
-        errs() << "\n";
-      else
-        errs() << " with \"" << R.getReplacementText() << "\"\n";
-    }
-  }
-}
-
-// FIXME: Remove this function after changing clang-apply-replacements to use
-// Replacements class.
-bool applyAllReplacements(const std::vector<tooling::Replacement> &Replaces,
-                          Rewriter &Rewrite) {
-  bool Result = true;
-  for (auto I = Replaces.begin(), E = Replaces.end(); I != E; ++I) {
-    if (I->isApplicable()) {
-      Result = I->apply(Rewrite) && Result;
-    } else {
-      Result = false;
-    }
-  }
-  return Result;
-}
-
-// FIXME: moved from libToolingCore. remove this when std::vector<Replacement>
-// is replaced with tooling::Replacements class.
-static void deduplicate(std::vector<tooling::Replacement> &Replaces,
-                        std::vector<tooling::Range> &Conflicts) {
-  if (Replaces.empty())
-    return;
-
-  auto LessNoPath = [](const tooling::Replacement &LHS,
-                       const tooling::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 tooling::Replacement &LHS,
-                        const tooling::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
-  tooling::Range ConflictRange(Replaces.front().getOffset(),
-                               Replaces.front().getLength());
-  unsigned ConflictStart = 0;
-  unsigned ConflictLength = 1;
-  for (unsigned i = 1; i < Replaces.size(); ++i) {
-    tooling::Range Current(Replaces[i].getOffset(), Replaces[i].getLength());
-    if (ConflictRange.overlapsWith(Current)) {
-      // Extend conflicted range
-      ConflictRange =
-          tooling::Range(ConflictRange.getOffset(),
-                         std::max(ConflictRange.getLength(),
-                                  Current.getOffset() + Current.getLength() -
-                                      ConflictRange.getOffset()));
-      ++ConflictLength;
-    } else {
-      if (ConflictLength > 1)
-        Conflicts.push_back(tooling::Range(ConflictStart, ConflictLength));
-      ConflictRange = Current;
-      ConflictStart = i;
-      ConflictLength = 1;
-    }
-  }
-
-  if (ConflictLength > 1)
-    Conflicts.push_back(tooling::Range(ConflictStart, ConflictLength));
-}
-
-/// \brief Deduplicates and tests for conflicts among the replacements for each
-/// file in \c Replacements. Any conflicts found are reported.
+/// \param[in] TUs Collection of all found and deserialized
+/// TranslationUnitReplacements.
+/// \param[in] TUDs Collection of all found and deserialized
+/// TranslationUnitDiagnostics.
+/// \param[in] SM Used to deduplicate paths.
 ///
-/// \post Replacements[i].getOffset() <= Replacements[i+1].getOffset().
-///
-/// \param[in,out] Replacements Container of all replacements grouped by file
-/// to be deduplicated and checked for conflicts.
-/// \param[in] SM SourceManager required for conflict reporting.
-///
-/// \returns \parblock
-///          \li true if conflicts were detected
-///          \li false if no conflicts were detected
-static bool deduplicateAndDetectConflicts(FileToReplacementsMap &Replacements,
-                                          SourceManager &SM) {
-  bool conflictsFound = false;
-
-  for (auto &FileAndReplacements : Replacements) {
-    const FileEntry *Entry = FileAndReplacements.first;
-    auto &Replacements = FileAndReplacements.second;
-    assert(Entry != nullptr && "No file entry!");
-
-    std::vector<tooling::Range> Conflicts;
-    deduplicate(FileAndReplacements.second, Conflicts);
-
-    if (Conflicts.empty())
-      continue;
-
-    conflictsFound = true;
-
-    errs() << "There are conflicting changes to " << Entry->getName() << ":\n";
-
-    for (const tooling::Range &Conflict : Conflicts) {
-      auto ConflictingReplacements = llvm::makeArrayRef(
-          &Replacements[Conflict.getOffset()], Conflict.getLength());
-      reportConflict(Entry, ConflictingReplacements, SM);
-    }
-  }
-
-  return conflictsFound;
-}
-
-bool mergeAndDeduplicate(const TUReplacements &TUs,
-                         FileToReplacementsMap &GroupedReplacements,
-                         clang::SourceManager &SM) {
-
-  // Group all replacements by target file.
+/// \returns A map mapping FileEntry to a set of Replacement targeting that
+/// file.
+static llvm::DenseMap<const FileEntry *, std::vector<tooling::Replacement>>
+groupReplacements(const TUReplacements &TUs, const TUDiagnostics &TUDs,
+                  const clang::SourceManager &SM) {
   std::set<StringRef> Warned;
-  for (const auto &TU : TUs) {
-    for (const tooling::Replacement &R : TU.Replacements) {
-      // Use the file manager to deduplicate paths. FileEntries are
-      // automatically canonicalized.
-      if (const FileEntry *Entry = SM.getFileManager().getFile(R.getFilePath())) {
-        GroupedReplacements[Entry].push_back(R);
-      } else if (Warned.insert(R.getFilePath()).second) {
-          errs() << "Described file '" << R.getFilePath()
-                 << "' doesn't exist. Ignoring...\n";
-      }
+  llvm::DenseMap<const FileEntry *, std::vector<tooling::Replacement>>
+      GroupedReplacements;
+
+  auto AddToGroup = [&](const tooling::Replacement &R) {
+    // Use the file manager to deduplicate paths. FileEntries are
+    // automatically canonicalized.
+    if (const FileEntry *Entry = SM.getFileManager().getFile(R.getFilePath())) {
+      GroupedReplacements[Entry].push_back(R);
+    } else if (Warned.insert(R.getFilePath()).second) {
+      errs() << "Described file '" << R.getFilePath()
+             << "' doesn't exist. Ignoring...\n";
     }
+  };
+
+  for (const auto &TU : TUs)
+    for (const tooling::Replacement &R : TU.Replacements)
+      AddToGroup(R);
+
+  for (const auto &TU : TUDs)
+    for (const auto &D : TU.Diagnostics)
+      for (const auto &Fix : D.Fix)
+        for (const tooling::Replacement &R : Fix.second)
+          AddToGroup(R);
+
+  // Sort replacements per file to keep consistent behavior when
+  // clang-apply-replacements run on differents machine.
+  for (auto &FileAndReplacements : GroupedReplacements) {
+    llvm::sort(FileAndReplacements.second.begin(),
+               FileAndReplacements.second.end());
   }
 
-  // Ask clang to deduplicate and report conflicts.
-  return !deduplicateAndDetectConflicts(GroupedReplacements, SM);
+  return GroupedReplacements;
 }
 
-bool mergeAndDeduplicate(const TUDiagnostics &TUs,
-                         FileToReplacementsMap &GroupedReplacements,
+bool mergeAndDeduplicate(const TUReplacements &TUs, const TUDiagnostics &TUDs,
+                         FileToChangesMap &FileChanges,
                          clang::SourceManager &SM) {
+  auto GroupedReplacements = groupReplacements(TUs, TUDs, SM);
+  bool ConflictDetected = false;
 
-  // Group all replacements by target file.
-  std::set<StringRef> Warned;
-  for (const auto &TU : TUs) {
-    for (const auto &D : TU.Diagnostics) {
-      for (const auto &Fix : D.Fix) {
-        for (const tooling::Replacement &R : Fix.second) {
-          // Use the file manager to deduplicate paths. FileEntries are
-          // automatically canonicalized.
-          if (const FileEntry *Entry = SM.getFileManager().getFile(R.getFilePath())) {
-            GroupedReplacements[Entry].push_back(R);
-          } else if (Warned.insert(R.getFilePath()).second) {
-              errs() << "Described file '" << R.getFilePath()
-                     << "' doesn't exist. Ignoring...\n";
-          }
-        }
+  // To report conflicting replacements on corresponding file, all replacements
+  // are stored into 1 big AtomicChange.
+  for (const auto &FileAndReplacements : GroupedReplacements) {
+    const FileEntry *Entry = FileAndReplacements.first;
+    const SourceLocation BeginLoc =
+        SM.getLocForStartOfFile(SM.getOrCreateFileID(Entry, SrcMgr::C_User));
+    tooling::AtomicChange FileChange(Entry->getName(), Entry->getName());
+    for (const auto &R : FileAndReplacements.second) {
+      llvm::Error Err =
+          FileChange.replace(SM, BeginLoc.getLocWithOffset(R.getOffset()),
+                             R.getLength(), R.getReplacementText());
+      if (Err) {
+        // FIXME: This will report conflicts by pair using a file+offset format
+        // which is not so much human readable.
+        // A first improvement could be to translate offset to line+col. For
+        // this and without loosing error message some modifications arround
+        // `tooling::ReplacementError` are need (access to
+        // `getReplacementErrString`).
+        // A better strategy could be to add a pretty printer methods for
+        // conflict reporting. Methods that could be parameterized to report a
+        // conflict in different format, file+offset, file+line+col, or even
+        // more human readable using VCS conflict markers.
+        // For now, printing directly the error reported by `AtomicChange` is
+        // the easiest solution.
+        errs() << llvm::toString(std::move(Err)) << "\n";
+        ConflictDetected = true;
       }
     }
+    FileChanges.try_emplace(Entry,
+                            std::vector<tooling::AtomicChange>{FileChange});
   }
 
-  // Ask clang to deduplicate and report conflicts.
-  return !deduplicateAndDetectConflicts(GroupedReplacements, SM);
-}
-
-bool applyReplacements(const FileToReplacementsMap &GroupedReplacements,
-                       clang::Rewriter &Rewrites) {
-
-  // Apply all changes
-  //
-  // FIXME: No longer certain GroupedReplacements is really the best kind of
-  // data structure for applying replacements. Rewriter certainly doesn't care.
-  // However, until we nail down the design of ReplacementGroups, might as well
-  // leave this as is.
-  for (const auto &FileAndReplacements : GroupedReplacements) {
-    if (!applyAllReplacements(FileAndReplacements.second, Rewrites))
-      return false;
-  }
-
-  return true;
-}
-
-RangeVector calculateChangedRanges(
-    const std::vector<clang::tooling::Replacement> &Replaces) {
-  RangeVector ChangedRanges;
-
-  // Generate the new ranges from the replacements.
-  int Shift = 0;
-  for (const tooling::Replacement &R : Replaces) {
-    unsigned Offset = R.getOffset() + Shift;
-    unsigned Length = R.getReplacementText().size();
-    Shift += Length - R.getLength();
-    ChangedRanges.push_back(tooling::Range(Offset, Length));
-  }
-
-  return ChangedRanges;
+  return !ConflictDetected;
 }
 
-bool writeFiles(const clang::Rewriter &Rewrites) {
-
-  for (auto BufferI = Rewrites.buffer_begin(), BufferE = Rewrites.buffer_end();
-       BufferI != BufferE; ++BufferI) {
-    StringRef FileName =
-        Rewrites.getSourceMgr().getFileEntryForID(BufferI->first)->getName();
-
-    std::error_code EC;
-    llvm::raw_fd_ostream FileStream(FileName, EC, llvm::sys::fs::F_Text);
-    if (EC) {
-      errs() << "Warning: Could not write to " << EC.message() << "\n";
-      continue;
-    }
-    BufferI->second.write(FileStream);
-  }
-
-  return true;
+llvm::Expected<std::string>
+applyChanges(StringRef File, const std::vector<tooling::AtomicChange> &Changes,
+             const tooling::ApplyChangesSpec &Spec,
+             DiagnosticsEngine &Diagnostics) {
+  FileManager Files((FileSystemOptions()));
+  SourceManager SM(Diagnostics, Files);
+
+  llvm::ErrorOr<std::unique_ptr<MemoryBuffer>> Buffer =
+      SM.getFileManager().getBufferForFile(File);
+  if (!Buffer)
+    return errorCodeToError(Buffer.getError());
+  return tooling::applyAtomicChanges(File, Buffer.get()->getBuffer(), Changes,
+                                     Spec);
 }
 
 bool deleteReplacementFiles(const TUReplacementFiles &Files,
index bee5579..94d1f43 100644 (file)
@@ -12,6 +12,7 @@ target_link_libraries(clang-apply-replacements
   clangFormat
   clangRewrite
   clangToolingCore
+  clangToolingRefactor
   )
 
 install(TARGETS clang-apply-replacements
index 1382755..7f97b8f 100644 (file)
@@ -86,116 +86,6 @@ static void printVersion(raw_ostream &OS) {
   OS << "clang-apply-replacements version " CLANG_VERSION_STRING << "\n";
 }
 
-/// \brief Convenience function to get rewritten content for \c Filename from
-/// \c Rewrites.
-///
-/// \pre Replacements[i].getFilePath() == Replacements[i+1].getFilePath().
-/// \post Replacements.empty() -> Result.empty()
-///
-/// \param[in] Replacements Replacements to apply
-/// \param[in] Rewrites Rewriter to use to apply replacements.
-/// \param[out] Result Contents of the file after applying replacements if
-/// replacements were provided.
-///
-/// \returns \parblock
-///          \li true if all replacements were applied successfully.
-///          \li false if at least one replacement failed to apply.
-static bool
-getRewrittenData(const std::vector<tooling::Replacement> &Replacements,
-                 Rewriter &Rewrites, std::string &Result) {
-  if (Replacements.empty())
-    return true;
-
-  if (!applyAllReplacements(Replacements, Rewrites))
-    return false;
-
-  SourceManager &SM = Rewrites.getSourceMgr();
-  FileManager &Files = SM.getFileManager();
-
-  StringRef FileName = Replacements.begin()->getFilePath();
-  const clang::FileEntry *Entry = Files.getFile(FileName);
-  assert(Entry && "Expected an existing file");
-  FileID ID = SM.translateFile(Entry);
-  assert(ID.isValid() && "Expected a valid FileID");
-  const RewriteBuffer *Buffer = Rewrites.getRewriteBufferFor(ID);
-  Result = std::string(Buffer->begin(), Buffer->end());
-
-  return true;
-}
-
-/// \brief Apply \c Replacements and return the new file contents.
-///
-/// \pre Replacements[i].getFilePath() == Replacements[i+1].getFilePath().
-/// \post Replacements.empty() -> Result.empty()
-///
-/// \param[in] Replacements Replacements to apply.
-/// \param[out] Result Contents of the file after applying replacements if
-/// replacements were provided.
-/// \param[in] Diagnostics For diagnostic output.
-///
-/// \returns \parblock
-///          \li true if all replacements applied successfully.
-///          \li false if at least one replacement failed to apply.
-static bool
-applyReplacements(const std::vector<tooling::Replacement> &Replacements,
-                  std::string &Result, DiagnosticsEngine &Diagnostics) {
-  FileManager Files((FileSystemOptions()));
-  SourceManager SM(Diagnostics, Files);
-  Rewriter Rewrites(SM, LangOptions());
-
-  return getRewrittenData(Replacements, Rewrites, Result);
-}
-
-/// \brief Apply code formatting to all places where replacements were made.
-///
-/// \pre !Replacements.empty().
-/// \pre Replacements[i].getFilePath() == Replacements[i+1].getFilePath().
-/// \pre Replacements[i].getOffset() <= Replacements[i+1].getOffset().
-///
-/// \param[in] Replacements Replacements that were made to the file. Provided
-/// to indicate where changes were made.
-/// \param[in] FileData The contents of the file \b after \c Replacements have
-/// been applied.
-/// \param[out] FormattedFileData The contents of the file after reformatting.
-/// \param[in] FormatStyle Style to apply.
-/// \param[in] Diagnostics For diagnostic output.
-///
-/// \returns \parblock
-///          \li true if reformatting replacements were all successfully
-///          applied.
-///          \li false if at least one reformatting replacement failed to apply.
-static bool
-applyFormatting(const std::vector<tooling::Replacement> &Replacements,
-                const StringRef FileData, std::string &FormattedFileData,
-                const format::FormatStyle &FormatStyle,
-                DiagnosticsEngine &Diagnostics) {
-  assert(!Replacements.empty() && "Need at least one replacement");
-
-  RangeVector Ranges = calculateChangedRanges(Replacements);
-
-  StringRef FileName = Replacements.begin()->getFilePath();
-  tooling::Replacements R =
-      format::reformat(FormatStyle, FileData, Ranges, FileName);
-
-  // FIXME: Remove this copy when tooling::Replacements is implemented as a
-  // vector instead of a set.
-  std::vector<tooling::Replacement> FormattingReplacements;
-  std::copy(R.begin(), R.end(), back_inserter(FormattingReplacements));
-
-  if (FormattingReplacements.empty()) {
-    FormattedFileData = FileData;
-    return true;
-  }
-
-  FileManager Files((FileSystemOptions()));
-  SourceManager SM(Diagnostics, Files);
-  SM.overrideFileContents(Files.getFile(FileName),
-                          llvm::MemoryBuffer::getMemBufferCopy(FileData));
-  Rewriter Rewrites(SM, LangOptions());
-
-  return getRewrittenData(FormattingReplacements, Rewrites, FormattedFileData);
-}
-
 int main(int argc, char **argv) {
   cl::HideUnrelatedOptions(makeArrayRef(VisibleCategories));
 
@@ -244,34 +134,23 @@ int main(int argc, char **argv) {
   FileManager Files((FileSystemOptions()));
   SourceManager SM(Diagnostics, Files);
 
-  FileToReplacementsMap GroupedReplacements;
-  if (!mergeAndDeduplicate(TURs, GroupedReplacements, SM))
-    return 1;
-  if (!mergeAndDeduplicate(TUDs, GroupedReplacements, SM))
+  FileToChangesMap Changes;
+  if (!mergeAndDeduplicate(TURs, TUDs, Changes, SM))
     return 1;
 
-  Rewriter ReplacementsRewriter(SM, LangOptions());
-
-  for (const auto &FileAndReplacements : GroupedReplacements) {
-    // This shouldn't happen but if a file somehow has no replacements skip to
-    // next file.
-    if (FileAndReplacements.second.empty())
-      continue;
-
-    std::string NewFileData;
-    StringRef FileName = FileAndReplacements.first->getName();
-    if (!applyReplacements(FileAndReplacements.second, NewFileData,
-                           Diagnostics)) {
-      errs() << "Failed to apply replacements to " << FileName << "\n";
-      continue;
-    }
-
-    // Apply formatting if requested.
-    if (DoFormat &&
-        !applyFormatting(FileAndReplacements.second, NewFileData, NewFileData,
-                         FormatStyle, Diagnostics)) {
-      errs() << "Failed to apply reformatting replacements for " << FileName
-             << "\n";
+  tooling::ApplyChangesSpec Spec;
+  Spec.Cleanup = true;
+  Spec.Style = FormatStyle;
+  Spec.Format = DoFormat ? tooling::ApplyChangesSpec::kAll
+                         : tooling::ApplyChangesSpec::kNone;
+
+  for (const auto &FileChange : Changes) {
+    const FileEntry *Entry = FileChange.first;
+    StringRef FileName = Entry->getName();
+    llvm::Expected<std::string> NewFileData =
+        applyChanges(FileName, FileChange.second, Spec, Diagnostics);
+    if (!NewFileData) {
+      errs() << llvm::toString(NewFileData.takeError()) << "\n";
       continue;
     }
 
@@ -282,8 +161,7 @@ int main(int argc, char **argv) {
       llvm::errs() << "Could not open " << FileName << " for writing\n";
       continue;
     }
-
-    FileStream << NewFileData;
+    FileStream << *NewFileData;
   }
 
   return 0;
index 52d2fe8..8f6f65a 100644 (file)
@@ -6,8 +6,8 @@ Diagnostics:
     FilePath: $(path)/basic.h
     FileOffset: 148
     Replacements:
-      - FilePath:        $(path)/basic.h
-        Offset:          148
-        Length:          0
-        ReplacementText: 'override '
+      - FilePath:        $(path)/../basic/basic.h
+        Offset:          298
+        Length:          1
+        ReplacementText: elem
 ...
index 32dddd3..b66f577 100644 (file)
@@ -1,11 +1,12 @@
-There are conflicting changes to $(path)/common.h:
-The following changes conflict:
-  Replace 8:8-8:33 with "auto & i : ints"
-  Replace 8:8-8:33 with "int & elem : ints"
-The following changes conflict:
-  Replace 9:5-9:11 with "elem"
-  Replace 9:5-9:11 with "i"
-The following changes conflict:
-  Remove 12:3-12:14
-  Insert at 12:12 (int*)
-  Replace 12:12-12:12 with "nullptr"
+The new replacement overlaps with an existing replacement.
+New replacement: $(path)/common.h: 106:+26:"int & elem : ints"
+Existing replacement: $(path)/common.h: 106:+26:"auto & i : ints"
+The new replacement overlaps with an existing replacement.
+New replacement: $(path)/common.h: 140:+7:"i"
+Existing replacement: $(path)/common.h: 140:+7:"elem"
+The new replacement overlaps with an existing replacement.
+New replacement: $(path)/common.h: 169:+0:"(int*)"
+Existing replacement: $(path)/common.h: 160:+12:""
+The new replacement overlaps with an existing replacement.
+New replacement: $(path)/common.h: 169:+1:"nullptr"
+Existing replacement: $(path)/common.h: 160:+12:""
diff --git a/clang-tools-extra/test/clang-apply-replacements/Inputs/identical/file1.yaml b/clang-tools-extra/test/clang-apply-replacements/Inputs/identical/file1.yaml
new file mode 100644 (file)
index 0000000..1ef5651
--- /dev/null
@@ -0,0 +1,18 @@
+---
+MainSourceFile:     identical.cpp
+Diagnostics:
+  - DiagnosticName: test-identical-insertion
+    Message: Fix
+    FilePath: $(path)/identical.cpp
+    FileOffset: 12
+    Replacements:
+      - FilePath:        $(path)/identical.cpp
+        Offset:          12
+        Length:          0
+        ReplacementText: '0'
+      - FilePath:        $(path)/identical.cpp
+        Offset:          12
+        Length:          0
+        ReplacementText: '0'
+...
+
diff --git a/clang-tools-extra/test/clang-apply-replacements/Inputs/identical/identical.cpp b/clang-tools-extra/test/clang-apply-replacements/Inputs/identical/identical.cpp
new file mode 100644 (file)
index 0000000..bdaab4f
--- /dev/null
@@ -0,0 +1,2 @@
+class MyType {};
+// CHECK: class MyType00 {};
diff --git a/clang-tools-extra/test/clang-apply-replacements/Inputs/order-dependent/expected.txt b/clang-tools-extra/test/clang-apply-replacements/Inputs/order-dependent/expected.txt
new file mode 100644 (file)
index 0000000..cf38b34
--- /dev/null
@@ -0,0 +1,3 @@
+The new insertion has the same insert location as an existing replacement.
+New replacement: $(path)/order-dependent.cpp: 12:+0:"1"
+Existing replacement: $(path)/order-dependent.cpp: 12:+0:"0"
diff --git a/clang-tools-extra/test/clang-apply-replacements/Inputs/order-dependent/file1.yaml b/clang-tools-extra/test/clang-apply-replacements/Inputs/order-dependent/file1.yaml
new file mode 100644 (file)
index 0000000..a823438
--- /dev/null
@@ -0,0 +1,13 @@
+---
+MainSourceFile:     order-dependent.cpp
+Diagnostics:
+  - DiagnosticName: test-order-dependent-insertion
+    Message: Fix
+    FilePath: $(path)/order-dependent.cpp
+    FileOffset: 12
+    Replacements:
+      - FilePath:        $(path)/order-dependent.cpp
+        Offset:          12
+        Length:          0
+        ReplacementText: '0'
+...
diff --git a/clang-tools-extra/test/clang-apply-replacements/Inputs/order-dependent/file2.yaml b/clang-tools-extra/test/clang-apply-replacements/Inputs/order-dependent/file2.yaml
new file mode 100644 (file)
index 0000000..634d3ca
--- /dev/null
@@ -0,0 +1,13 @@
+---
+MainSourceFile:     order-dependent.cpp
+Diagnostics:
+  - DiagnosticName: test-order-dependent-insertion
+    Message: Fix
+    FilePath: $(path)/order-dependent.cpp
+    FileOffset: 12
+    Replacements:
+      - FilePath:        $(path)/order-dependent.cpp
+        Offset:          12
+        Length:          0
+        ReplacementText: '1'
+...
diff --git a/clang-tools-extra/test/clang-apply-replacements/Inputs/order-dependent/order-dependent.cpp b/clang-tools-extra/test/clang-apply-replacements/Inputs/order-dependent/order-dependent.cpp
new file mode 100644 (file)
index 0000000..5b98860
--- /dev/null
@@ -0,0 +1 @@
+class MyType {};
diff --git a/clang-tools-extra/test/clang-apply-replacements/identical.cpp b/clang-tools-extra/test/clang-apply-replacements/identical.cpp
new file mode 100644 (file)
index 0000000..b513f3e
--- /dev/null
@@ -0,0 +1,5 @@
+// RUN: mkdir -p %T/Inputs/identical
+// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/identical/identical.cpp > %T/Inputs/identical/identical.cpp
+// RUN: sed "s#\$(path)#%/T/Inputs/identical#" %S/Inputs/identical/file1.yaml > %T/Inputs/identical/file1.yaml
+// RUN: clang-apply-replacements %T/Inputs/identical
+// RUN: FileCheck -input-file=%T/Inputs/identical/identical.cpp %S/Inputs/identical/identical.cpp
diff --git a/clang-tools-extra/test/clang-apply-replacements/order-dependent.cpp b/clang-tools-extra/test/clang-apply-replacements/order-dependent.cpp
new file mode 100644 (file)
index 0000000..769f4f7
--- /dev/null
@@ -0,0 +1,7 @@
+// RUN: mkdir -p %T/Inputs/order-dependent
+// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/order-dependent/order-dependent.cpp > %T/Inputs/order-dependent/order-dependent.cpp
+// RUN: sed "s#\$(path)#%/T/Inputs/order-dependent#" %S/Inputs/order-dependent/file1.yaml > %T/Inputs/order-dependent/file1.yaml
+// RUN: sed "s#\$(path)#%/T/Inputs/order-dependent#" %S/Inputs/order-dependent/file2.yaml > %T/Inputs/order-dependent/file2.yaml
+// RUN: sed "s#\$(path)#%/T/Inputs/order-dependent#" %S/Inputs/order-dependent/expected.txt > %T/Inputs/order-dependent/expected.txt
+// RUN: not clang-apply-replacements %T/Inputs/order-dependent > %T/Inputs/order-dependent/output.txt 2>&1
+// RUN: diff -b %T/Inputs/order-dependent/output.txt %T/Inputs/order-dependent/expected.txt
index 39f5b6f..85aaebf 100644 (file)
@@ -9,6 +9,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "clang-apply-replacements/Tooling/ApplyReplacements.h"
+#include "clang/Format/Format.h"
 #include "gtest/gtest.h"
 
 using namespace clang::replace;
@@ -41,11 +42,12 @@ TEST(ApplyReplacementsTest, mergeDiagnosticsWithNoFixes) {
       IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()), DiagOpts.get());
   FileManager Files((FileSystemOptions()));
   SourceManager SM(Diagnostics, Files);
+  TUReplacements TURs;
   TUDiagnostics TUs =
       makeTUDiagnostics("path/to/source.cpp", "diagnostic", {}, {}, "path/to");
-  FileToReplacementsMap ReplacementsMap;
+  FileToChangesMap ReplacementsMap;
 
-  EXPECT_TRUE(mergeAndDeduplicate(TUs, ReplacementsMap, SM));
+  EXPECT_TRUE(mergeAndDeduplicate(TURs, TUs, ReplacementsMap, SM));
   EXPECT_TRUE(ReplacementsMap.empty());
 }
 
index 180aef1..9e5fac2 100644 (file)
@@ -9,7 +9,6 @@ include_directories(
 
 add_extra_unittest(ClangApplyReplacementsTests
   ApplyReplacementsTest.cpp
-  ReformattingTest.cpp
   )
 
 target_link_libraries(ClangApplyReplacementsTests
@@ -17,4 +16,5 @@ target_link_libraries(ClangApplyReplacementsTests
   clangApplyReplacements
   clangBasic
   clangToolingCore
+  clangToolingRefactor
   )
diff --git a/clang-tools-extra/unittests/clang-apply-replacements/ReformattingTest.cpp b/clang-tools-extra/unittests/clang-apply-replacements/ReformattingTest.cpp
deleted file mode 100644 (file)
index 48083ed..0000000
+++ /dev/null
@@ -1,67 +0,0 @@
-//===- clang-apply-replacements/ReformattingTest.cpp ----------------------===//
-//
-//                     The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-
-#include "clang-apply-replacements/Tooling/ApplyReplacements.h"
-#include "common/VirtualFileHelper.h"
-#include "clang/Format/Format.h"
-#include "clang/Tooling/Refactoring.h"
-#include "gtest/gtest.h"
-
-using namespace clang;
-using namespace clang::tooling;
-using namespace clang::replace;
-
-typedef std::vector<clang::tooling::Replacement> ReplacementsVec;
-
-static Replacement makeReplacement(unsigned Offset, unsigned Length,
-                                   unsigned ReplacementLength,
-                                   llvm::StringRef FilePath = "") {
-  return Replacement(FilePath, Offset, Length,
-                     std::string(ReplacementLength, '~'));
-}
-
-// generate a set of replacements containing one element
-static ReplacementsVec makeReplacements(unsigned Offset, unsigned Length,
-                                        unsigned ReplacementLength,
-                                        llvm::StringRef FilePath = "~") {
-  ReplacementsVec Replaces;
-  Replaces.push_back(
-      makeReplacement(Offset, Length, ReplacementLength, FilePath));
-  return Replaces;
-}
-
-// Put these functions in the clang::tooling namespace so arg-dependent name
-// lookup finds these functions for the EXPECT_EQ macros below.
-namespace clang {
-namespace tooling {
-
-std::ostream &operator<<(std::ostream &os, const Range &R) {
-  return os << "Range(" << R.getOffset() << ", " << R.getLength() << ")";
-}
-
-} // namespace tooling
-} // namespace clang
-
-// Ensure zero-length ranges are produced. Even lines where things are deleted
-// need reformatting.
-TEST(CalculateChangedRangesTest, producesZeroLengthRange) {
-  RangeVector Changes = calculateChangedRanges(makeReplacements(0, 4, 0));
-  EXPECT_EQ(Range(0, 0), Changes.front());
-}
-
-// Basic test to ensure replacements turn into ranges properly.
-TEST(CalculateChangedRangesTest, calculatesRanges) {
-  ReplacementsVec R;
-  R.push_back(makeReplacement(2, 0, 3));
-  R.push_back(makeReplacement(5, 2, 4));
-  RangeVector Changes = calculateChangedRanges(R);
-
-  Range ExpectedRanges[] = { Range(2, 3), Range(8, 4) };
-  EXPECT_TRUE(std::equal(Changes.begin(), Changes.end(), ExpectedRanges));
-}