[PATCH] D31176: [clang-rename] Support renaming qualified symbol

Eric Liu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Mar 24 09:03:34 PDT 2017


ioeric added inline comments.


================
Comment at: clang-rename/USRLocFinder.cpp:195
+// Find all locations identified by the given USRs. Traverse the AST and find
+// every AST node whose USR is in the given USRs' set.
+class RenameLocFinder
----------------
hokein wrote:
> ioeric wrote:
> > I think this also does some renaming?
> No, this class is only responsible for finding rename locations and other information which are used for renaming. The renaming stuff is done by `USRSymbolRenamer`.
Sure. I meant you should also document it.


================
Comment at: clang-rename/USRLocFinder.cpp:359
+
+  // Returns a list of using declarations which are needed to update.
+  const std::vector<const UsingDecl *> &getUsingDecls() const {
----------------
hokein wrote:
> ioeric wrote:
> > I think these are using shadows only?
> These are interested `UsingDecl`s which contain `UsingShadowDecl` of the symbol declarations being renamed.
Sure. But maybe also document it?


================
Comment at: clang-rename/USRLocFinder.h:36
+/// \return Replacement for renaming.
+std::vector<tooling::Replacement>
+createRenameReplacement(llvm::ArrayRef<std::string> USRs,
----------------
hokein wrote:
> ioeric wrote:
> > Why use `std::vector` instead of `tooling::Replacements`?
> Seems that we don't get many benefits from using `tooling::Replacements` here. This function could be called multiple times (for renaming multiple symbols), we need to merge/add all replacements in caller side. if using `tooling::Replacements`, we will merge twice (one is in the API implementation).
I think what we really want is `AtomicChange`. We shouldn't be using `std::vector` or `std::set` replacements anymore.


https://reviews.llvm.org/D31176





More information about the cfe-commits mailing list