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

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL287929: Do not do raw name replacement when FromDecl is a 
class forward-declaration. (authored by ioeric).

Changed prior to commit:
  https://reviews.llvm.org/D27132?vs=79307=79308#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D27132

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

Index: cfe/trunk/unittests/Tooling/LookupTest.cpp
===
--- cfe/trunk/unittests/Tooling/LookupTest.cpp
+++ cfe/trunk/unittests/Tooling/LookupTest.cpp
@@ -13,23 +13,30 @@
 
 namespace {
 struct GetDeclsVisitor : TestVisitor {
-  std::function OnCall;
+  std::function OnCall = [&](CallExpr *Expr) {};
+  std::function OnRecordTypeLoc = [&](RecordTypeLoc Type) {
+  };
   SmallVector 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(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: cfe/trunk/lib/Tooling/Core/Lookup.cpp
===
--- cfe/trunk/lib/Tooling/Core/Lookup.cpp
+++ cfe/trunk/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,20 @@
  "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, so we can't do a
+  // raw name replacement in this case.
   const bool class_name_only = !Use;
   const bool in_global_namespace =
   isa(FromDecl->getDeclContext());
-  if (class_name_only && !in_global_namespace &&
+  const bool is_class_forward_decl =
+  isa(FromDecl) &&
+  !cast(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

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

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 79307.
ioeric marked an inline comment as done.
ioeric added a comment.

- Address comments.


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 {
-  std::function OnCall;
+  std::function OnCall = [&](CallExpr *Expr) {};
+  std::function OnRecordTypeLoc = [&](RecordTypeLoc Type) {
+  };
   SmallVector 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(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,20 @@
  "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, so we can't do a
+  // raw name replacement in this case.
   const bool class_name_only = !Use;
   const bool in_global_namespace =
   isa(FromDecl->getDeclContext());
-  if (class_name_only && !in_global_namespace &&
+  const bool is_class_forward_decl =
+  isa(FromDecl) &&
+  !cast(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

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

[Benjamin Kramer via Phabricator via cfe-commits]

bkramer accepted this revision.
bkramer added inline comments.
This revision is now accepted and ready to land.



Comment at: lib/Tooling/Core/Lookup.cpp:131
+  // 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.

the "given the nature" part doesn't add any useful information. Either drop it 
or explain in more detail what's the problem with forward decls.


https://reviews.llvm.org/D27132



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

[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 {
-  std::function OnCall;
+  std::function OnCall = [&](CallExpr *Expr) {};
+  std::function OnRecordTypeLoc = [&](RecordTypeLoc Type) {
+  };
   SmallVector 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(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(FromDecl->getDeclContext());
-  if (class_name_only && !in_global_namespace &&
+  const bool is_class_forward_decl =
+  isa(FromDecl) &&
+  !cast(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