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


================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:207
+  llvm::DenseMap<const FileEntry *, std::set<tooling::Replacement, LessNoPath>>
+      GroupedReplacements;
+
----------------
jdemeule wrote:
> 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.
`AtomicChange` does deduplicate identical replacements but not insertions, because it wouldn't know whether users really want the insertions to be deduplicated or not (e.g. imagine a tool wants to insert two `)` at the same location). So it doesn't support that test case intentionally. In general, users (i.e. tools generating changes) are responsible for ensuring changes are deduplicated/applied in the expected way by using the correct interface (e.g. `replace`, `insert` etc).  I think it would probably make more sense to change the test to deduplicate identical non-insertion replacements. It would also make sense to add another test case where identical insertions are both applied.

For more semantics of conflicting/duplicated replacements, see https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Core/Replacement.h#L217 


================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:279
+  if (!NewCode) {
+    errs() << "Failed to apply fixes on " << File << "\n";
+    return false;
----------------
jdemeule wrote:
> 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?
> I will use llvm::Expected as you suggest.
I think `NewCode` is already `llvm::Expected<std::string>`. You would only need to explicitly handle the error.

> Do you have some ideas to made a test failed at this level?
That's a good question. I think we would really need unit tests for the `ApplyReplacements` library in order to get better coverage. But it's probably out of the scope of this patch, so I'd also be fine without the test. Up to you :)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43764