[PATCH] D43764: [clang-apply-replacements] Convert tooling::Replacements to tooling::AtomicChange for conflict resolving of changes, code cleanup, and code formatting.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

This looks great! Just a few nits left.



================
Comment at: clang-apply-replacements/include/clang-apply-replacements/Tooling/ApplyReplacements.h:75
+/// \brief Deduplicate, check for conflicts, and convert all Replacements stored
+/// in \c TUs to AtomicChange. If conflicts occur, no Replacements are applied.
 ///
----------------
` If conflicts occur, no Replacements are applied.` This doesn't seem to be accurate; non-conflicting replacements are still added.


================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:173
 
-  // Group all replacements by target file.
-  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 = groupReplacements(TUs, TUDs, SM);
----------------
nit: I'd probably use `auto GroupedReplacements = ...;`.


================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:179
+  // To keep current clang-apply-replacement behavior, report conflicting
+  // replacements skip file containing conflicts, all replacements are stored
+  // into 1 big AtomicChange.
----------------
I think we are only skipping conflicts; non-conflicting replacements are still added even if the file contains other conflicts?

This doesn't seem to have caused any problem because the current caller simply drops all changes if conflict is detected in any file, but we should still make it clear what the behavior of `mergeAndDeduplicate` is (in the API doc) e.g.  what would `FileChanges` contain if there is conflict in some but not all files?


================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:218
+             DiagnosticsEngine &Diagnostics) {
 
+  FileManager Files((FileSystemOptions()));
----------------
nit: redundant empty line.


================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:227
 
-  return true;
+  StringRef Code = Buffer.get()->getBuffer();
+  return tooling::applyAtomicChanges(File, Code, Changes, Spec);
----------------
nit: consider inline this to save one variable.


https://reviews.llvm.org/D43764