[clang-rename] Don't add prefix qualifiers to the declaration and definition of the renamed symbol.

[ioeric]

Lg with nits.

lib/Tooling/Refactoring/Rename/USRLocFinder.cpp

163 ↗

(On Diff #118362)

s/are known to/should be/ ?

166 ↗

(On Diff #118362)

maybe "... for both symbol references and symbol definitions", just for clarity?

443 ↗

(On Diff #118362)

nit: no braces

[hokein]

Address review comments.

cfe/trunk/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp

struct RenameInfo {
     // The end location of a symbol being renamed.
     SourceLocation End;
     // The declaration of a symbol being renamed (can be nullptr).
     const NamedDecl *FromDecl;
     // The declaration in which the nested name is contained (can be nullptr).
     const Decl *Context;
     // The nested name being replaced (can be nullptr).
     const NestedNameSpecifier *Specifier;
+    // Determine whether the prefix qualifiers of the NewName should be ignored.
+    // Normally, we set it to true for the symbol declaration and definition to
+    // avoid adding prefix qualifiers.
+    // For example, if it is true and NewName is "a::b::foo", then the symbol
+    // occurrence which the RenameInfo points to will be renamed to "foo".
+    bool IgnorePrefixQualifers;
   };
 
-  // FIXME: Currently, prefix qualifiers will be added to the renamed symbol
-  // definition (e.g. "class Foo {};" => "class b::Bar {};" when renaming
-  // "a::Foo" to "b::Bar").
-  // For renaming declarations/definitions, prefix qualifiers should be filtered
-  // out.
   bool VisitNamedDecl(const NamedDecl *Decl) {
     // UsingDecl has been handled in other place.
     if (llvm::isa<UsingDecl>(Decl))
       return true;
 
     // DestructorDecl has been handled in Typeloc.
     if (llvm::isa<CXXDestructorDecl>(Decl))
       return true;
 
     if (Decl->isImplicit())
       return true;
 
     if (isInUSRSet(Decl)) {
-      RenameInfo Info = {Decl->getLocation(), Decl->getLocation(), nullptr,
-                         nullptr, nullptr};
+      RenameInfo Info = {Decl->getLocation(),
+                         Decl->getLocation(),
+                         /*FromDecl=*/nullptr,
+                         /*Context=*/nullptr,
+                         /*Specifier=*/nullptr,
+                         /*IgnorePrefixQualifers=*/true};
       RenameInfos.push_back(Info);
     }
     return true;
   }
 
   bool VisitDeclRefExpr(const DeclRefExpr *Expr) {
     const NamedDecl *Decl = Expr->getFoundDecl();
     if (isInUSRSet(Decl)) {
       RenameInfo Info = {Expr->getSourceRange().getBegin(),
-                         Expr->getSourceRange().getEnd(), Decl,
-                         getClosestAncestorDecl(*Expr), Expr->getQualifier()};
+                         Expr->getSourceRange().getEnd(),
+                         Decl,
+                         getClosestAncestorDecl(*Expr),
+                         Expr->getQualifier(),
+                         /*IgnorePrefixQualifers=*/false};
       RenameInfos.push_back(Info);
     }
 
     return true;
   }
 
   bool VisitUsingDecl(const UsingDecl *Using) {
     for (const auto *UsingShadow : Using->shadows()) {
bool VisitNestedNameSpecifierLocations(NestedNameSpecifierLoc NestedLoc) {
     if (IsTypeAliasWhichWillBeRenamedElsewhere(NestedLoc.getTypeLoc()))
       return true;
 
     if (const auto *TargetDecl =
             getSupportedDeclFromTypeLoc(NestedLoc.getTypeLoc())) {
       if (isInUSRSet(TargetDecl)) {
         RenameInfo Info = {NestedLoc.getBeginLoc(),
                            EndLocationForType(NestedLoc.getTypeLoc()),
-                           TargetDecl, getClosestAncestorDecl(NestedLoc),
-                           NestedLoc.getNestedNameSpecifier()->getPrefix()};
+                           TargetDecl,
+                           getClosestAncestorDecl(NestedLoc),
+                           NestedLoc.getNestedNameSpecifier()->getPrefix(),
+                           /*IgnorePrefixQualifers=*/false};
         RenameInfos.push_back(Info);
       }
     }
     return true;
   }
 
   bool VisitTypeLoc(TypeLoc Loc) {
     if (IsTypeAliasWhichWillBeRenamedElsewhere(Loc))
if (const auto *TargetDecl = getSupportedDeclFromTypeLoc(Loc)) {
         //   ElaboratedType 0x33b9390 'a::Foo' sugar
         //   `-RecordType 0x338fef0 'class a::Foo'
         //     `-CXXRecord 0x338fe58 'Foo'
         //
         // Skip if this is an inner typeLoc.
         if (!ParentTypeLoc.isNull() &&
             isInUSRSet(getSupportedDeclFromTypeLoc(ParentTypeLoc)))
           return true;
-        RenameInfo Info = {StartLocationForType(Loc), EndLocationForType(Loc),
-                           TargetDecl, getClosestAncestorDecl(Loc),
-                           GetNestedNameForType(Loc)};
+        RenameInfo Info = {StartLocationForType(Loc),
+                           EndLocationForType(Loc),
+                           TargetDecl,
+                           getClosestAncestorDecl(Loc),
+                           GetNestedNameForType(Loc),
+                           /*IgnorePrefixQualifers=*/false};
         RenameInfos.push_back(Info);
         return true;
       }
     }
 
     // Handle specific template class specialiation cases.
     if (const auto *TemplateSpecType =
             dyn_cast<TemplateSpecializationType>(Loc.getType())) {
if (const auto *TemplateSpecType =
         // For the qualified template class specification type like
         // "ns::Foo<int>" in "ns::Foo<int>& f();", we want the parent typeLoc
         // (ElaboratedType) of the TemplateSpecializationType in order to
         // catch the prefix qualifiers "ns::".
         if (!ParentTypeLoc.isNull() &&
             llvm::isa<ElaboratedType>(ParentTypeLoc.getType()))
           TargetLoc = ParentTypeLoc;
         RenameInfo Info = {
-            StartLocationForType(TargetLoc), EndLocationForType(TargetLoc),
+            StartLocationForType(TargetLoc),
+            EndLocationForType(TargetLoc),
             TemplateSpecType->getTemplateName().getAsTemplateDecl(),
             getClosestAncestorDecl(
                 ast_type_traits::DynTypedNode::create(TargetLoc)),
-            GetNestedNameForType(TargetLoc)};
+            GetNestedNameForType(TargetLoc),
+            /*IgnorePrefixQualifers=*/false};
         RenameInfos.push_back(Info);
       }
     }
     return true;
   }
 
   // Returns a list of RenameInfo.
   const std::vector<RenameInfo> &getRenameInfos() const { return RenameInfos; }
if (Err) {
                    << llvm::toString(std::move(Err)) << "\n";
       return;
     }
     AtomicChanges.push_back(std::move(ReplaceChange));
   };
 
   for (const auto &RenameInfo : Finder.getRenameInfos()) {
     std::string ReplacedName = NewName.str();
+    if (RenameInfo.IgnorePrefixQualifers) {
+      // Get the name without prefix qualifiers from NewName.
+      size_t LastColonPos = NewName.find_last_of(':');
+      if (LastColonPos != std::string::npos)
+        ReplacedName = NewName.substr(LastColonPos + 1);
+    } else {
       if (RenameInfo.FromDecl && RenameInfo.Context) {
         if (!llvm::isa<clang::TranslationUnitDecl>(
                 RenameInfo.Context->getDeclContext())) {
           ReplacedName = tooling::replaceNestedName(
               RenameInfo.Specifier, RenameInfo.Context->getDeclContext(),
               RenameInfo.FromDecl,
-            NewName.startswith("::") ? NewName.str() : ("::" + NewName).str());
+              NewName.startswith("::") ? NewName.str()
+                                       : ("::" + NewName).str());
         }
       }
       // If the NewName contains leading "::", add it back.
       if (NewName.startswith("::") && NewName.substr(2) == ReplacedName)
         ReplacedName = NewName.str();
+    }
     Replace(RenameInfo.Begin, RenameInfo.End, ReplacedName);
   }
 
   // Hanlde using declarations explicitly as "using a::Foo" don't trigger
   // typeLoc for "a::Foo".
   for (const auto *Using : Finder.getUsingDecls())
     Replace(Using->getLocStart(), Using->getLocEnd(), "using " + NewName.str());
 
   return AtomicChanges;
 }
 
 } // end namespace tooling
 } // end namespace clang

cfe/trunk/unittests/Rename/RenameClassTest.cpp

std::string Expected = R"(
        private:
         New* next_;
       };
     )";
   std::string After = runClangRenameOnCode(Before, "Old", "New");
   CompareSnippets(Expected, After);
 }
 
-// FIXME: no prefix qualifiers being added to the class definition and
-// constructor.
 TEST_F(ClangRenameTest, RenameClassWithNamespaceWithInlineMembers) {
   std::string Before = R"(
       namespace ns {
       class Old {
        public:
         Old() {}
         ~Old() {}
 
         Old* next() { return next_; }
 
        private:
         Old* next_;
       };
       }  // namespace ns
     )";
   std::string Expected = R"(
       namespace ns {
-      class ns::New {
+      class New {
        public:
-        ns::New() {}
+        New() {}
         ~New() {}
 
         New* next() { return next_; }
 
        private:
         New* next_;
       };
       }  // namespace ns
     )";
   std::string After = runClangRenameOnCode(Before, "ns::Old", "ns::New");
   CompareSnippets(Expected, After);
 }
 
-// FIXME: no prefix qualifiers being added to the class definition and
-// constructor.
 TEST_F(ClangRenameTest, RenameClassWithNamespaceWithOutOfInlineMembers) {
   std::string Before = R"(
       namespace ns {
       class Old {
        public:
         Old();
         ~Old();
 
         Old* next();
 
        private:
         Old* next_;
       };
 
       Old::Old() {}
       Old::~Old() {}
       Old* Old::next() { return next_; }
       }  // namespace ns
     )";
   std::string Expected = R"(
       namespace ns {
-      class ns::New {
+      class New {
        public:
-        ns::New();
+        New();
         ~New();
 
         New* next();
 
        private:
         New* next_;
       };
 
-      New::ns::New() {}
+      New::New() {}
       New::~New() {}
       New* New::next() { return next_; }
       }  // namespace ns
     )";
   std::string After = runClangRenameOnCode(Before, "ns::Old", "ns::New");
   CompareSnippets(Expected, After);
 }
 
-// FIXME: no prefix qualifiers being added to the definition.
 TEST_F(ClangRenameTest, RenameClassInInheritedConstructor) {
   // `using Base::Base;` will generate an implicit constructor containing usage
   // of `::ns::Old` which should not be matched.
   std::string Before = R"(
       namespace ns {
+      class Old;
       class Old {
         int x;
       };
       class Base {
        protected:
         Old *moo_;
        public:
         Base(Old *moo) : moo_(moo) {}
       };
       class Derived : public Base {
        public:
          using Base::Base;
       };
       }  // namespace ns
       int main() {
         ::ns::Old foo;
         ::ns::Derived d(&foo);
         return 0;
       })";
   std::string Expected = R"(
       namespace ns {
-      class ns::New {
+      class New;
+      class New {
         int x;
       };
       class Base {
        protected:
         New *moo_;
        public:
         Base(New *moo) : moo_(moo) {}
       };
std::string Before = R"(
       void f() {
         S s1, s2, s3;
         // This causes an implicit assignment operator to be created.
         s1 = s2 = s3;
       }
       )";
   std::string Expected = R"(
       namespace ns {
-      class ::new_ns::New {
+      class New {
       };
       } // namespace ns
       struct S {
         int y;
         ::new_ns::New old;
       };
       void f() {
         S s1, s2, s3;
         // This causes an implicit assignment operator to be created.
         s1 = s2 = s3;
       }
       )";
   std::string After = runClangRenameOnCode(Before, "ns::Old", "::new_ns::New");
   CompareSnippets(Expected, After);
 }
 
-// FIXME: no prefix qualifiers being adding to the definition.
 TEST_F(ClangRenameTest, ReferencesInLambdaFunctionParameters) {
   std::string Before = R"(
       template <class T>
       class function;
       template <class R, class... ArgTypes>
       class function<R(ArgTypes...)> {
       public:
         template <typename Functor>
std::string Expected = R"(
         function(Functor f) {}
 
         function() {}
 
         R operator()(ArgTypes...) const {}
       };
 
       namespace ns {
-      class ::new_ns::New {};
+      class New {};
       void f() {
         function<void(::new_ns::New)> func;
       }
       }  // namespace ns)";
   std::string After = runClangRenameOnCode(Before, "ns::Old", "::new_ns::New");
   CompareSnippets(Expected, After);
 }
 
 } // anonymous namespace
 } // namespace test
 } // namespace clang_rename
 } // namesdpace clang