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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
