[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.

Thanks for the cleanup! This looks really nice!



================
Comment at: clang-apply-replacements/include/clang-apply-replacements/Tooling/ApplyReplacements.h:45
+/// \brief Map mapping file name to AtomicChange targeting that file.
+typedef llvm::DenseMap<const clang::FileEntry *, tooling::AtomicChange>
+    FileToChangeMap;
----------------
I would expect this to be a map from files to a group of AtomicChanges (e.g. `std::vector<AtomicChange>`). In general, a single AtomicChange contains changes around a single code location which are likely to conflict with each other, and either all changes or no change is applied. A file usually corresponds to a set of atomic changes. 

Intuitively, I think clang-apply-replacements should simple gather a set of atomic changes for each file and let `applyAtomicChanges` handle the conflicts, but its current behavior is to skip conflicting replacements and keep applying other replacements. This is not ideal, but I guess I understand where this came from, and we might not be able to fix this in this patch since most tools produce replacements instead of AtomicChange.

I would still suggest make this a map something like `llvm::DenseMap<const clang::FileEntry *, std::vector<tooling::AtomicChange>>`. To preserve the current behavior (i.e. skip conflicts), when you group all replacements for a single file, you would still put all replacements into a single AtomicChange, but when you actually put the change into the map, you put it as a vector of a single change.


================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:207
+  llvm::DenseMap<const FileEntry *, std::set<tooling::Replacement, LessNoPath>>
+      GroupedReplacements;
+
----------------
I don't think we need to do the deduplication here anymore. AtomicChange handles duplicates for you. I think all you need to do here is to group replacements by files and later convert replacements to atomic changes.


================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:248
+    const auto BeginLoc = SM.getLocForStartOfFile(FID);
+    tooling::AtomicChange Change(Entry->getName(), Entry->getName());
+    for (const auto &R : FileAndReplacements.second)
----------------
(Related to my comment on `FileToChangeMap`)

We should add FIXME here explaining why we are putting all replacements for a single file into one AtomicChange (e.g. we want to be able to detect and report each individual conflict) instead of a set of changes.


================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:259
+      }
+    Changes.insert(std::make_pair(Entry, std::move(Change)));
   }
----------------
(Also related to my comment on FileToChangeMap)

This would be something like

```
Changes.insert(std::make_pair(Entry, {Change}));
```


================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:279
+  if (!NewCode) {
+    errs() << "Failed to apply fixes on " << File << "\n";
+    return false;
----------------
You should handle the error in `llvm::Expected`. You could convert it to string and add to the error message with `llvm::toString(NewCode.takeError())`. It would be nice if we could have a test case for such cases.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43764