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

[Jeremy Demeule via Phabricator via cfe-commits]

jdemeule added a comment.

Thanks for your feedback, they are very precious to me!



================
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;
----------------
ioeric wrote:
> 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.
I got your point. I will update the patch this way.


================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:207
+  llvm::DenseMap<const FileEntry *, std::set<tooling::Replacement, LessNoPath>>
+      GroupedReplacements;
+
----------------
ioeric wrote:
> 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.
I think I wrongly use AtomicChange somewhere because it doesn't deduplicate same replacement automatically.
For exemple, in the test suite, basic test defines 2 time the same replacement (adding 'override ' at offset 148) and I do not manage to avoid AtomicChange to add 'override override '. This is why I have kept the deduplicate step.


================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:279
+  if (!NewCode) {
+    errs() << "Failed to apply fixes on " << File << "\n";
+    return false;
----------------
ioeric wrote:
> 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.
I will use `llvm::Expected` as you suggest.
Do you have some ideas to made a test failed at this level?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43764