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

Reply via email to