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


================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:207
+  llvm::DenseMap<const FileEntry *, std::set<tooling::Replacement, LessNoPath>>
+      GroupedReplacements;
+
----------------
ioeric wrote:
> 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 
That's make sense indeed. My confusion comes from I do not want to break existing tests. I will update them the way you describe.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43764