ioeric added inline comments.
================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:353
+ const FileEntry *Entry = FileAndReplacements.first;
+ ReplacementsToAtomicChanges DeduplicatedChanges(SM, Entry);
+ for (const auto &R : FileAndReplacements.second)
----------------
jdemeule wrote:
> ioeric wrote:
> > Sorry that I didn't make myself clear... what you had in the previous
> > revision was correct (for the current use case of apply-all-replacements)
> > i.e. store all replacements in one `AtomicChange`, which allows you to
> > detect conflicts on the fly. So the code here can be simplified as:
> >
> > ```
> > ...
> > Entry = ...;
> > AtomicChange FileChange;
> > for (const auto &R : FileAndReplacements.second) {
> > auto Err = FileChange.replace( <args from R> );
> > if (Err)
> > reportConflict(Entry, std::move(Err)); // reportConflict as we go
> > }
> > FileChanges.emplace(Entry, {FileChange});
> > ...
> > ```
> >
> > I think with this set up, you wouldn't need `ReplacementsToAtomicChanges`,
> > `ConflictError` and `reportConflicts`.
> I think I have over-interpreting your previous comment :-)
> However, I am still confused about error reporting.
> Currently, clang-apply-replacements reports conflicting replacement per file
> and per conflict.
> For example:
> ```
> There are conflicting changes to XXX:
> The following changes conflict:
> Replace 8:8-8:33 with "AAA"
> Replace 8:8-8:33 with "BBB"
> Remove 8:10-8:15
> Insert at 8:14 CCC
> ```
>
> With this patch, conflict are reported by pair (first replacement/conflicted
> one) and I am able to print the corresponding file only once (thanks to the
> defered reporting).
> ```
> There are conflicting changes to XXX:
> The following changes conflict:
> Replace 8:8-8:33 with "AAA"
> Replace 8:8-8:33 with "BBB"
> The following changes conflict:
> Replace 8:8-8:33 with "AAA"
> Remove 8:10-8:15
> The following changes conflict:
> Replace 8:8-8:33 with "AAA"
> Insert at 8:14 CCC
> ```
>
> I prefer the way you suggest to report conflict but we will loose or print
> conflicting file at each conflict detected.
> I even more prefer to use `llvm::toString(std::move(Err))` but this will
> fully change the reporting and will also be reported by pair.
> ```
> The new replacement overlaps with an existing replacement.
> New replacement: XXX: 106:+26:"AAA"
> Existing replacement: XXX: 106:+26:"BBB"
> The new replacement overlaps with an existing replacement.
> New replacement: XXX: 106:+26:"AAA"
> Existing replacement: XXX: 112:+0:"CCC"
> The new replacement overlaps with an existing replacement.
> New replacement: XXX: 106:+26:"AAA"
> Existing replacement: XXX: 108:+12:""
> ```
I am inclined to changing the current behavior and reporting by pairs because
1) this would simplify the code a lot.because we could basically reuse the
conflict detection from replacement library.
2) IMO, pairs are easier to read than grouping multiple conflicts - users would
still need to interpret the conflicts and figure out how all replacements
conflict in a group, while reporting as pairs already gives you this
information.
3) in practice, it's uncommon to see more than two conflicting replacements at
the same code location.
I would vote for the last approach (with `llvm::toString(std::move(Err))`)
which is easy to implement and doesn't really regress the `UI` that much. WDYT?
https://reviews.llvm.org/D43764
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
