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

[Eric Liu via Phabricator via cfe-commits]

ioeric 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:
> 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`.


https://reviews.llvm.org/D27054