[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:41
+  /// \brief Creates an edit list for a key position.
+  EditList(const SourceManager &SM, SourceLocation KeyPosition);
+
----------------
ioeric wrote:
> ioeric wrote:
> > klimek wrote:
> > > ioeric wrote:
> > > > djasper wrote:
> > > > > I wonder whether we should always use a SourceLocation as key or whether we want to leave this up to the users. E.g. we could make this take a string parameter and provide a
> > > > > 
> > > > >   string getKeyForLocation(const SourceManager &SM, SourceLocation KeyPosition);
> > > > I think the key idea of EditList is that it groups a set of edits around a key position. For refactoring, using position as key makes sense to me (since all edits will be around some position), and I couldn't think of other things that are good to be a key here.
> > > An idea would be an edit that just inserts multiple headers, but does not directly touch a source location (thus, we might want to key things off of files?).
> > We could make the key position the start of the file in this case. But looks like a customize-able key could potentially enable more opportunities, I'll go with Daniel's suggestion. Thanks!
> So I tried the `getKeyForLocation` way, but we still need `FilePath` in the constructor to fully initialize an EditList. 
> 
> I think it might be better if we keep the current constructor (since this will be the most-commonly used one IMO) and adds another constructor that takes `FilePath` and `Key`.
Ah, but you might want to include more details in the Key, e.g. the branch or source revision. My point is that the user might be better equipped to know what a valid key might be. But I am happy to keep it as is and add another constructor where we pass in a key when needed.


================
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:
> > 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.


================
Comment at: lib/Tooling/Refactoring/EditList.cpp:164
+
+llvm::Error EditList::insertAfter(const SourceManager &SM, SourceLocation Loc,
+                                  llvm::StringRef Text) {
----------------
Don't duplicate this code. Add an internal function that takes an extra parameter that controls conflict resolution.


https://reviews.llvm.org/D27054