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