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: > > jdemeule wrote: > > > 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 > > > ``` > > > > > The conflict marker format looks promising! > > > > You are right regarding offset vs line+column location. I agree offset is > > less readable. I think we could probably build offset -> line+column > > translation into `ReplacementError` if necessary. For example, we could add > > a pretty-print method/helper that takes in source code and outputs readable > > error messages with offset translated to line+columns according to the > > source code, which is basically what the current `replaceConflict` function > > does. Ideally, that would also handle/consume the underlying errors so that > > we could switch to use that instead of `llvm::toString`. > Do you prefer to add the transformation offset -> line+column in this patch > or in a future one (with an option to switch between line report and conflict > marker for example)? I think it would be easier to have those in a different patch. For this patch, reporting conflict pairs with either offsets or line+column (by using the transformation that is already there) would be fine (with a FIXME maybe). (These are all great improvements! Thanks for doing this!) https://reviews.llvm.org/D43764 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits