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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
