[PATCH] D43764: [clang-apply-replacements] Convert tooling::Replacements to tooling::AtomicChange for conflict resolving of changes, code cleanup, and code formatting.

[Jeremy Demeule via Phabricator via cfe-commits]

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