[clang-move] Handle helpers with forward declarations.

Reviewers: ioeric

Reviewed By: ioeric

Subscribers: cfe-commits

Differential Revision: https://reviews.llvm.org/D28801

llvm-svn: 292215

clang-tools-extra/clang-move/ClangMove.cpp

@@ -553,16 +553,22 @@ void ClangMoveTool::registerMatchers(ast_matchers::MatchFinder *Finder) {
 
   // Matchers for helper declarations in old.cc.
   auto InAnonymousNS = hasParent(namespaceDecl(isAnonymous()));
-  auto DefinitionInOldCC = allOf(isDefinition(), unless(InMovedClass), InOldCC);
-  auto IsOldCCHelperDefinition =
-      allOf(DefinitionInOldCC, anyOf(isStaticStorageClass(), InAnonymousNS));
+  auto NotInMovedClass= allOf(unless(InMovedClass), InOldCC);
+  auto IsOldCCHelper =
+      allOf(NotInMovedClass, anyOf(isStaticStorageClass(), InAnonymousNS));
   // Match helper classes separately with helper functions/variables since we
   // want to reuse these matchers in finding helpers usage below.
-  auto HelperFuncOrVar =
-      namedDecl(notInMacro(), anyOf(functionDecl(IsOldCCHelperDefinition),
-                                    varDecl(IsOldCCHelperDefinition)));
+  //
+  // There could be forward declarations usage for helpers, especially for
+  // classes and functions. We need include these forward declarations.
+  //
+  // Forward declarations for variable helpers will be excluded as these
+  // declarations (with "extern") are not supposed in cpp file.
+   auto HelperFuncOrVar =
+      namedDecl(notInMacro(), anyOf(functionDecl(IsOldCCHelper),
+                                    varDecl(isDefinition(), IsOldCCHelper)));
   auto HelperClasses =
-      cxxRecordDecl(notInMacro(), DefinitionInOldCC, InAnonymousNS);
+      cxxRecordDecl(notInMacro(), NotInMovedClass, InAnonymousNS);
   // Save all helper declarations in old.cc.
   Finder->addMatcher(
       namedDecl(anyOf(HelperFuncOrVar, HelperClasses)).bind("helper_decls"),
@@ -650,6 +656,8 @@ void ClangMoveTool::run(const ast_matchers::MatchFinder::MatchResult &Result) {
                  Result.Nodes.getNodeAs<clang::NamedDecl>("helper_decls")) {
     MovedDecls.push_back(ND);
     HelperDeclarations.push_back(ND);
+    DEBUG(llvm::dbgs() << "Add helper : "
+                       << ND->getNameAsString() << " (" << ND << ")\n");
   } else if (const auto *UD =
                  Result.Nodes.getNodeAs<clang::NamedDecl>("using_decl")) {
     MovedDecls.push_back(UD);
@@ -703,9 +711,12 @@ void ClangMoveTool::removeDeclsInOldFiles() {
     // We remove the helper declarations which are not used in the old.cc after
     // moving the given declarations.
     for (const auto *D : HelperDeclarations) {
-      if (!UsedDecls.count(HelperDeclRGBuilder::getOutmostClassOrFunDecl(D))) {
+      DEBUG(llvm::dbgs() << "Check helper is used: "
+                         << D->getNameAsString() << " (" << D << ")\n");
+      if (!UsedDecls.count(HelperDeclRGBuilder::getOutmostClassOrFunDecl(
+              D->getCanonicalDecl()))) {
         DEBUG(llvm::dbgs() << "Helper removed in old.cc: "
-                           << D->getNameAsString() << " " << D << "\n");
+                           << D->getNameAsString() << " (" << D << ")\n");
         RemovedDecls.push_back(D);
       }
     }
@@ -781,7 +792,8 @@ void ClangMoveTool::moveDeclsToNewFiles() {
   // given symbols being moved.
   for (const auto *D : NewCCDecls) {
     if (llvm::is_contained(HelperDeclarations, D) &&
-        !UsedDecls.count(HelperDeclRGBuilder::getOutmostClassOrFunDecl(D)))
+        !UsedDecls.count(HelperDeclRGBuilder::getOutmostClassOrFunDecl(
+            D->getCanonicalDecl())))
       continue;
 
     DEBUG(llvm::dbgs() << "Helper used in new.cc: " << D->getNameAsString()

clang-tools-extra/clang-move/HelperDeclRefGraph.cpp

@@ -10,8 +10,11 @@
 #include "HelperDeclRefGraph.h"
 #include "ClangMove.h"
 #include "clang/AST/Decl.h"
+#include "llvm/Support/Debug.h"
 #include <vector>
 
+#define DEBUG_TYPE "clang-move"
+
 namespace clang {
 namespace move {
 
@@ -113,13 +116,19 @@ void HelperDeclRGBuilder::run(
   if (const auto *FuncRef = Result.Nodes.getNodeAs<DeclRefExpr>("func_ref")) {
     const auto *DC = Result.Nodes.getNodeAs<Decl>("dc");
     assert(DC);
-
-    RG->addEdge(getOutmostClassOrFunDecl(DC->getCanonicalDecl()),
-                getOutmostClassOrFunDecl(FuncRef->getDecl()));
+    DEBUG(llvm::dbgs() << "Find helper function usage: "
+                       << FuncRef->getDecl()->getNameAsString() << " ("
+                       << FuncRef->getDecl() << ")\n");
+    RG->addEdge(
+        getOutmostClassOrFunDecl(DC->getCanonicalDecl()),
+        getOutmostClassOrFunDecl(FuncRef->getDecl()->getCanonicalDecl()));
   } else if (const auto *UsedClass =
                  Result.Nodes.getNodeAs<CXXRecordDecl>("used_class")) {
     const auto *DC = Result.Nodes.getNodeAs<Decl>("dc");
     assert(DC);
+    DEBUG(llvm::dbgs() << "Find helper class usage: "
+                       << UsedClass->getNameAsString() << " (" << UsedClass
+                       << ")\n");
     RG->addEdge(getOutmostClassOrFunDecl(DC->getCanonicalDecl()), UsedClass);
   }
 }

clang-tools-extra/test/clang-move/Inputs/helper_decls_test.cpp

@@ -76,3 +76,22 @@ static int HelperFun5() {
 void Fun1() { HelperFun5(); }
 
 } // namespace a
+
+namespace b {
+namespace {
+void HelperFun7();
+
+class HelperC4;
+} // namespace
+
+void Fun3() {
+  HelperFun7();
+  HelperC4 *t;
+}
+
+namespace {
+void HelperFun7() {}
+
+class HelperC4 {};
+} // namespace
+} // namespace b

clang-tools-extra/test/clang-move/Inputs/helper_decls_test.h

@@ -33,3 +33,7 @@ void Fun1();
 inline void Fun2() {}
 
 } // namespace a
+
+namespace b {
+void Fun3();
+} // namespace b

clang-tools-extra/test/clang-move/move-used-helper-decls.cpp

@@ -259,12 +259,43 @@
 
 // CHECK-OLD-FUN2-H-NOT: inline void Fun2() {}
 
+// ----------------------------------------------------------------------------
+// Test moving used helper function and its transively used functions.
+// ----------------------------------------------------------------------------
+// RUN: cp %S/Inputs/helper_decls_test*  %T/used-helper-decls/
+// RUN: clang-move -names="b::Fun3" -new_cc=%T/used-helper-decls/new_helper_decls_test.cpp -new_header=%T/used-helper-decls/new_helper_decls_test.h -old_cc=%T/used-helper-decls/helper_decls_test.cpp -old_header=../used-helper-decls/helper_decls_test.h %T/used-helper-decls/helper_decls_test.cpp -- -std=c++11
+// RUN: FileCheck -input-file=%T/used-helper-decls/new_helper_decls_test.cpp -check-prefix=CHECK-NEW-FUN3-CPP %s
+// RUN: FileCheck -input-file=%T/used-helper-decls/helper_decls_test.cpp -check-prefix=CHECK-OLD-FUN3-CPP %s
+
+// CHECK-NEW-FUN3-CPP: #include "{{.*}}new_helper_decls_test.h"
+// CHECK-NEW-FUN3-CPP-SAME: {{[[:space:]]}}
+// CHECK-NEW-FUN3-CPP-NEXT: namespace b {
+// CHECK-NEW-FUN3-CPP-NEXT: namespace {
+// CHECK-NEW-FUN3-CPP-NEXT: void HelperFun7();
+// CHECK-NEW-FUN3-CPP-SAME: {{[[:space:]]}}
+// CHECK-NEW-FUN3-CPP-NEXT: class HelperC4;
+// CHECK-NEW-FUN3-CPP-NEXT: } // namespace
+// CHECK-NEW-FUN3-CPP-SAME: {{[[:space:]]}}
+// CHECK-NEW-FUN3-CPP-NEXT: void Fun3() {
+// CHECK-NEW-FUN3-CPP-NEXT:   HelperFun7();
+// CHECK-NEW-FUN3-CPP-NEXT:   HelperC4 *t;
+// CHECK-NEW-FUN3-CPP-NEXT: }
+// CHECK-NEW-FUN3-CPP-NEXT: namespace {
+// CHECK-NEW-FUN3-CPP-NEXT: void HelperFun7() {}
+// CHECK-NEW-FUN3-CPP-SAME: {{[[:space:]]}}
+// CHECK-NEW-FUN3-CPP-NEXT: class HelperC4 {};
+// CHECK-NEW-FUN3-CPP-NEXT: } // namespace
+// CHECK-NEW-FUN3-CPP-NEXT: } // namespace b
+//
+// CHECK-OLD-FUN3-CPP-NOT: void HelperFun7();
+// CHECK-OLD-FUN3-CPP-NOT: void HelperFun7() {}
+// CHECK-OLD-FUN3-CPP-NOT: void Fun3() { HelperFun7(); }
 
 // ----------------------------------------------------------------------------
 // Test moving all symbols in headers.
 // ----------------------------------------------------------------------------
 // RUN: cp %S/Inputs/helper_decls_test*  %T/used-helper-decls/
-// RUN: clang-move -names="a::Class1, a::Class2, a::Class3, a::Class4, a::Class5, a::Class5, a::Class6, a::Class7, a::Fun1, a::Fun2" -new_cc=%T/used-helper-decls/new_helper_decls_test.cpp -new_header=%T/used-helper-decls/new_helper_decls_test.h -old_cc=%T/used-helper-decls/helper_decls_test.cpp -old_header=../used-helper-decls/helper_decls_test.h %T/used-helper-decls/helper_decls_test.cpp -- -std=c++11
+// RUN: clang-move -names="a::Class1, a::Class2, a::Class3, a::Class4, a::Class5, a::Class5, a::Class6, a::Class7, a::Fun1, a::Fun2, b::Fun3" -new_cc=%T/used-helper-decls/new_helper_decls_test.cpp -new_header=%T/used-helper-decls/new_helper_decls_test.h -old_cc=%T/used-helper-decls/helper_decls_test.cpp -old_header=../used-helper-decls/helper_decls_test.h %T/used-helper-decls/helper_decls_test.cpp -- -std=c++11
 // RUN: FileCheck -input-file=%T/used-helper-decls/new_helper_decls_test.h -check-prefix=CHECK-NEW-H %s
 // RUN: FileCheck -input-file=%T/used-helper-decls/new_helper_decls_test.cpp -check-prefix=CHECK-NEW-CPP %s
 // RUN: FileCheck -input-file=%T/used-helper-decls/helper_decls_test.h -allow-empty -check-prefix=CHECK-EMPTY %s
@@ -384,5 +415,24 @@
 // CHECK-NEW-CPP-NEXT: void Fun1() { HelperFun5(); }
 // CHECK-NEW-CPP-SAME: {{[[:space:]]}}
 // CHECK-NEW-CPP-NEXT: } // namespace a
+// CHECK-NEW-CPP-SAME: {{[[:space:]]}}
+// CHECK-NEW-CPP-NEXT: namespace b {
+// CHECK-NEW-CPP-NEXT: namespace {
+// CHECK-NEW-CPP-NEXT: void HelperFun7();
+// CHECK-NEW-CPP-SAME: {{[[:space:]]}}
+// CHECK-NEW-CPP-NEXT: class HelperC4;
+// CHECK-NEW-CPP-NEXT: } // namespace
+// CHECK-NEW-CPP-SAME: {{[[:space:]]}}
+// CHECK-NEW-CPP-NEXT: void Fun3() {
+// CHECK-NEW-CPP-NEXT:   HelperFun7();
+// CHECK-NEW-CPP-NEXT:   HelperC4 *t;
+// CHECK-NEW-CPP-NEXT: }
+// CHECK-NEW-CPP-SAME: {{[[:space:]]}}
+// CHECK-NEW-CPP-NEXT: namespace {
+// CHECK-NEW-CPP-NEXT: void HelperFun7() {}
+// CHECK-NEW-CPP-SAME: {{[[:space:]]}}
+// CHECK-NEW-CPP-NEXT: class HelperC4 {};
+// CHECK-NEW-CPP-NEXT: } // namespace
+// CHECK-NEW-CPP-NEXT: } // namespace b
 
 // CHECK-EMPTY: {{^}}{{$}}