[PATCH] D27132: Do not do raw name replacement when FromDecl is a class forward-declaration.

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.
ioeric added a reviewer: bkramer.
ioeric added a subscriber: cfe-commits.
Herald added a subscriber: klimek.

If the `FromDecl` is a class forward declaration, the reference is
still considered as referring to the original definition given the nature
of forward-declarations, so we can't do a raw name replacement in this case.


https://reviews.llvm.org/D27132

Files:
  lib/Tooling/Core/Lookup.cpp
  unittests/Tooling/LookupTest.cpp


Index: unittests/Tooling/LookupTest.cpp
===================================================================
--- unittests/Tooling/LookupTest.cpp
+++ unittests/Tooling/LookupTest.cpp
@@ -13,23 +13,30 @@
 
 namespace {
 struct GetDeclsVisitor : TestVisitor<GetDeclsVisitor> {
-  std::function<void(CallExpr *)> OnCall;
+  std::function<void(CallExpr *)> OnCall = [&](CallExpr *Expr) {};
+  std::function<void(RecordTypeLoc)> OnRecordTypeLoc = [&](RecordTypeLoc Type) {
+  };
   SmallVector<Decl *, 4> DeclStack;
 
   bool VisitCallExpr(CallExpr *Expr) {
     OnCall(Expr);
     return true;
   }
 
+  bool VisitRecordTypeLoc(RecordTypeLoc Loc) {
+    OnRecordTypeLoc(Loc);
+    return true;
+  }
+
   bool TraverseDecl(Decl *D) {
     DeclStack.push_back(D);
     bool Ret = TestVisitor::TraverseDecl(D);
     DeclStack.pop_back();
     return Ret;
   }
 };
 
-TEST(LookupTest, replaceNestedName) {
+TEST(LookupTest, replaceNestedFunctionName) {
   GetDeclsVisitor Visitor;
 
   auto replaceCallExpr = [&](const CallExpr *Expr,
@@ -121,4 +128,37 @@
                   "} } }\n");
 }
 
+TEST(LookupTest, replaceNestedClassName) {
+  GetDeclsVisitor Visitor;
+
+  auto replaceRecordTypeLoc = [&](RecordTypeLoc Loc,
+                                  StringRef ReplacementString) {
+    const auto *FD = cast<CXXRecordDecl>(Loc.getDecl());
+    return tooling::replaceNestedName(
+        nullptr, Visitor.DeclStack.back()->getDeclContext(), FD,
+        ReplacementString);
+  };
+
+  Visitor.OnRecordTypeLoc = [&](RecordTypeLoc Type) {
+    // Filter Types by name since there are other `RecordTypeLoc` in the test
+    // file.
+    if (Type.getDecl()->getQualifiedNameAsString() == "a::b::Foo")
+      EXPECT_EQ("x::Bar", replaceRecordTypeLoc(Type, "::a::x::Bar"));
+  };
+  Visitor.runOver("namespace a { namespace b {\n"
+                  "class Foo;\n"
+                  "namespace c { Foo f();; }\n"
+                  "} }\n");
+
+  Visitor.OnRecordTypeLoc = [&](RecordTypeLoc Type) {
+    // Filter Types by name since there are other `RecordTypeLoc` in the test
+    // file.
+    // `a::b::Foo` in using shadow decl is not `TypeLoc`.
+    if (Type.getDecl()->getQualifiedNameAsString() == "a::b::Foo")
+      EXPECT_EQ("Bar", replaceRecordTypeLoc(Type, "::a::x::Bar"));
+  };
+  Visitor.runOver("namespace a { namespace b { class Foo {}; } }\n"
+                  "namespace c { using a::b::Foo; Foo f();; }\n");
+}
+
 } // end anonymous namespace
Index: lib/Tooling/Core/Lookup.cpp
===================================================================
--- lib/Tooling/Core/Lookup.cpp
+++ lib/Tooling/Core/Lookup.cpp
@@ -13,6 +13,7 @@
 
 #include "clang/Tooling/Core/Lookup.h"
 #include "clang/AST/Decl.h"
+#include "clang/AST/DeclCXX.h"
 using namespace clang;
 using namespace clang::tooling;
 
@@ -121,14 +122,21 @@
          "Expected fully-qualified name!");
 
   // We can do a raw name replacement when we are not inside the namespace for
-  // the original function and it is not in the global namespace.  The
+  // the original class/function and it is not in the global namespace.  The
   // assumption is that outside the original namespace we must have a using
   // statement that makes this work out and that other parts of this refactor
-  // will automatically fix using statements to point to the new function
+  // will automatically fix using statements to point to the new class/function.
+  // However, if the `FromDecl` is a class forward declaration, the reference is
+  // still considered as referring to the original definition given the nature
+  // of forward-declarations, so we can't do a raw name replacement in this
+  // case.
   const bool class_name_only = !Use;
   const bool in_global_namespace =
       isa<TranslationUnitDecl>(FromDecl->getDeclContext());
-  if (class_name_only && !in_global_namespace &&
+  const bool is_class_forward_decl =
+      isa<CXXRecordDecl>(FromDecl) &&
+      !cast<CXXRecordDecl>(FromDecl)->isCompleteDefinition();
+  if (class_name_only && !in_global_namespace && !is_class_forward_decl &&
       !usingFromDifferentCanonicalNamespace(FromDecl->getDeclContext(),
                                             UseContext)) {
     auto Pos = ReplacementString.rfind("::");

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits