[PATCH] D27054: Introducing clang::tooling::EditList for refactoring tools.

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.


================
Comment at: include/clang/Tooling/Refactoring/EditList.h:82-94
+  /// \brief Adds a replacement that inserts \p Text at \p Loc. If this
+  /// insertion conflicts with an existing insertion (at the same position),
+  /// this will be inserted before the existing insertion. If the conflicting
+  /// replacement is not an insertion, an error is returned.
+  ///
+  /// \returns An llvm::Error carrying ReplacementError on error.
+  llvm::Error insertBefore(const SourceManager &SM, SourceLocation Loc,
----------------
ioeric wrote:
> djasper wrote:
> > ioeric wrote:
> > > djasper wrote:
> > > > Do we currently actually need these functions? They seem a bit dangerous.
> > > I think these functions are helpful. Insertion conflict is by far the most common conflict I've seen, and several users have asked for ways to resolve this. And generally, resolving such conflict is not straight-forward. Also notice that these interfaces only resolve insertion conflict, other conflicts (e.g. overlaps) will still raise errors.
> > > 
> > > Although inserting all texts at one location as a single replacement is preferred, I've seen several tools that use these in some way, i.e. generating all insertions at the same location is hard or impossible. 
> > I still have several concerns:
> > 1. This interface actually exposes two different layers: It has special function to insert text and headers and also the functions that add or merge replacements. IMO, that's not ideal.
> > 2. There is now no function insert() if users actually want to get the error for a conflict. Yes, they can then instead use addReplacement, but again, that's a completely separate layer of the interface.
> > 3. The logic of before after AND that this only applies to two conflicting insertions is a bit hard to explain.
> > 
> > Not sure what the right trade-off for all of these is. Maybe we should just remove addReplacement and mergeReplacements and instead add a replace() function? Maybe we should just have a single insert() function with an optional fourth parameter that can be used to control conflict resolution.
> `replace` + `insert` sounds like a good way to go. Do we want to support resolving general conflicts besides insertions though?
I think that might not be necessary. If so, we might need to see a few use cases to be sure to get it right.


https://reviews.llvm.org/D27054