[PATCH] D39332: [clang-refactor] Introduce a new rename rule for qualified symbols

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: lib/Tooling/Refactoring/Rename/RenamingAction.cpp:101
+                              std::string NewQualifiedName) {
+  return QualifiedRenameRule(std::move(OldQualifiedName),
+                             std::move(NewQualifiedName));
----------------
hokein wrote:
> arphaman wrote:
> > It might be better to find the declaration (and report error if needed) during in initiation, and then pass the `ND` to the class. Maybe then both `RenameOccurrences` and `QualifiedRenameRule` could subclass from one base class that actually does just this:
> > 
> > ``` 
> > auto USRs = getUSRsForDeclaration(ND, Context.getASTContext());
> >   assert(!USRs.empty());
> >   return tooling::createRenameAtomicChanges(
> >       USRs, NewQualifiedName, Context.getASTContext().getTranslationUnitDecl());
> > ```
> Done. Introduced a common interface `USRRenameRule`.
`USRRenameRule` doesn't seem to be a very useful abstraction. I think what Alex proposed is a base class that implements `createSourceReplacements` which could be shared by both `RenameOccurrences` and `QualifiedRenameRule`. Intuitively, un-qualified rename is a special case of qualified rename (maybe?), where namespace is not changed.


================
Comment at: lib/Tooling/Refactoring/Rename/RenamingAction.cpp:115
+      /*Description=*/
+      "Finds and renames qualified symbols in code with no indexer support",
+  };
----------------
We should also document the behavior when namespace is changed. What would happen to symbol definitions and symbol references?


https://reviews.llvm.org/D39332