[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);
+
----------------
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.


================
Comment at: include/clang/Tooling/Refactoring/EditList.h:43
+
+  EditList(std::string Key, std::string FilePath, std::string Error,
+           std::vector<std::string> InsertedHeaders,
----------------
djasper wrote:
> Does this need to be public? Specifically, couldn't the YamlTraits create a NormalizedEditList and then convertFromYAML can create the EditList object, which should have access to a private constructor?
You are right! Make this private now.


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


https://reviews.llvm.org/D27054