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

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 

  rCTE Clang Tools Extra

cfe-commits mailing list

Reply via email to