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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

I think this is a great start!

First round with some nits.



================
Comment at: clang-rename/RenamingAction.cpp:87
 
+class QualifiedRenamingASTConsumer : public ASTConsumer {
+public:
----------------
Comments. 


================
Comment at: clang-rename/RenamingAction.h:45
 
+class QualifiedRenamingAction {
+public:
----------------
Comments. Same for other classes.


================
Comment at: clang-rename/RenamingAction.h:60
+  const std::vector<std::string> &OldNames;
+  const std::vector<std::vector<std::string>> &USRList;
+  std::map<std::string, tooling::Replacements> &FileToReplaces;
----------------
What is `USRList`? 


================
Comment at: clang-rename/USRFinder.cpp:139
+      if (Name != Decl->getQualifiedNameAsString() &&
+          Name != "::" + Decl->getQualifiedNameAsString()) {
         return true;
----------------
nit: No braces for one-liners.


================
Comment at: clang-rename/USRFinder.cpp:200
 
+  // Also find all USRs of nested declarations.
+  NestedNameSpecifierLocFinder Finder(const_cast<ASTContext &>(Context));
----------------
It is unclear to me what `nested declarations` are.


================
Comment at: clang-rename/USRLocFinder.cpp:152
 
+clang::SourceLocation StartLocationForType(clang::TypeLoc TL) {
+  // For elaborated types (e.g. `struct a::A`) we want the portion after the
----------------
You don't need "clang::" here and elsewhere. 


================
Comment at: clang-rename/USRLocFinder.cpp:156
+  if (TL.getTypeLocClass() == clang::TypeLoc::Elaborated) {
+    clang::NestedNameSpecifierLoc nested_name_specifier =
+        TL.castAs<clang::ElaboratedTypeLoc>().getQualifierLoc();
----------------
Variable names should be LLVM style.


================
Comment at: clang-rename/USRLocFinder.cpp:157
+    clang::NestedNameSpecifierLoc nested_name_specifier =
+        TL.castAs<clang::ElaboratedTypeLoc>().getQualifierLoc();
+    if (nested_name_specifier.getNestedNameSpecifier())
----------------
Is this cast always safe? 


================
Comment at: clang-rename/USRLocFinder.cpp:169
+         TL.getTypeLocClass() == clang::TypeLoc::Qualified) {
+    TL = TL.getNextTypeLoc();
+  }
----------------
nit: No braces around one-liners. Same below.


================
Comment at: clang-rename/USRLocFinder.cpp:196
+
+class RenameLocFindingASTVisitor
+    : public clang::RecursiveASTVisitor<RenameLocFindingASTVisitor> {
----------------
Comments.


================
Comment at: clang-rename/USRLocFinder.cpp:196
+
+class RenameLocFindingASTVisitor
+    : public clang::RecursiveASTVisitor<RenameLocFindingASTVisitor> {
----------------
ioeric wrote:
> Comments.
I think this visitor deserves a file of its own.


================
Comment at: clang-rename/USRLocFinder.cpp:368
+private:
+  bool IsTypeAliasWhichWillBeRenamedElsewhere(TypeLoc TL) const {
+    while (!TL.isNull()) {
----------------
Note this is not expected behavior for alias types which would always be skipped in this check.


================
Comment at: clang-rename/USRLocFinder.cpp:466
+
+  for (const auto &Loc : Finder.getRenameInfos()) {
+    std::string ReplacedName = NewName.str();
----------------
`Loc` seems to be a bad name here. It is usually used for TypeLoc.


================
Comment at: clang-rename/USRLocFinder.h:34
+std::vector<NewNameReplacement>
+getQualifiedRenameLocsOfUSRs(llvm::ArrayRef<std::string> USRs,
+                             llvm::StringRef OldName, llvm::StringRef NewName,
----------------
Comments.


https://reviews.llvm.org/D31176