jdemeule 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) ---------------- ioeric wrote: > 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? I think if we can use `llvm::toString(std::move(Err))` for this patch, it will simplify the code and reuse already existing error message. Even if I found file+offset conflict location less readable than file+line+column. I have made some prototype to "enhance" error reporting but I think they should be put in dedicated patches and need further discutions. During my "research" I found we can use: * Conflict marker format. ``` /src/basic.h @@ 12:23-12:23 @@ <<<<<<< Existing replacement virtual void func() noexcept {} ======= virtual void func() override {} >>>>>>> New replacement ``` * A unified diff like. ``` /src/basic.h @@ 12:23-12:23 @@ - virtual void func() {} + virtual void func() noexcept {} + virtual void func() override {} ``` * A clang like diagnostic message. ``` /src/basic.h @@ 12:23-12:23 @@ virtual void func() {} ^ noexcept virtual void func() {} ^ override ``` https://reviews.llvm.org/D43764 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits