[clang-move] Handle helpers with forward declarations.

[ioeric]

Lgtm.

clang-move/ClangMove.cpp

if (!HasAnySymbolNames) {
     llvm::errs() << "No symbols being moved.\n";
     return;
   }
   auto InMovedClass =
       hasOutermostEnclosingClass(cxxRecordDecl(*HasAnySymbolNames));
 
   // 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.
+  //
+  // 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(IsOldCCHelperDefinition),
-                                    varDecl(IsOldCCHelperDefinition)));
+      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"),
       this);
 
   // Construct an AST-based call graph of helper declarations in old.cc.
   // In the following matcheres, "dc" is a caller while "helper_decls" and
   // "used_class" is a callee, so a new edge starting from caller to callee will
if (const auto *D =
     }
   } else if (const auto *ND =
                  Result.Nodes.getNodeAs<clang::NamedDecl>("static_decls")) {
     MovedDecls.push_back(ND);
   } else if (const auto *ND =
                  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);
   }
 }
 
 std::string ClangMoveTool::makeAbsolutePath(StringRef Path) {
   return MakeAbsolutePath(Context->OriginalRunningDirectory, Path);
if (!Context->Spec.OldHeader.empty()) {
     for (const auto *D : UnremovedDeclsInOldHeader)
       UnremovedDecls.push_back(D);
 
     auto UsedDecls = getUsedDecls(RGBuilder.getGraph(), UnremovedDecls);
 
     // 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);
       }
     }
   }
 
   for (const auto *RemovedDecl : RemovedDecls) {
     const auto &SM = RemovedDecl->getASTContext().getSourceManager();
     auto Range = getFullRange(RemovedDecl);
void ClangMoveTool::moveDeclsToNewFiles() {
   auto UsedDecls = getUsedDecls(RGBuilder.getGraph(), RemovedDecls);
   std::vector<const NamedDecl *> ActualNewCCDecls;
 
   // Filter out all unused helpers in NewCCDecls.
   // We only move the used helpers (including transively used helpers) and the
   // 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()
                        << " " << D << "\n");
     ActualNewCCDecls.push_back(D);
   }
 
   if (!Context->Spec.NewHeader.empty()) {

clang-move/HelperDeclRefGraph.cpp

 //===-- UsedHelperDeclFinder.cpp - AST-based call graph for helper decls --===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #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 {
 
 void HelperDeclRefGraph::print(raw_ostream &OS) const {
   OS << " --- Call graph Dump --- \n";
   for (auto I = DeclMap.begin(); I != DeclMap.end(); ++I) {
     const CallGraphNode *N = (I->second).get();
 
void HelperDeclRGBuilder::run(
   //
   //   int MoveClass::F() { int a = helper(); return a; }
   //
   // The matched "dc" of "helper" DeclRefExpr is a VarDecl, we traverse up AST
   // to find the outmost "MoveClass" CXXRecordDecl and use it as Caller.
   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);
   }
 }
 
 } // namespace move
 } // namespace clang

test/clang-move/Inputs/helper_decls_test.h

class Class7 {
   int g();
 };
 
 void Fun1();
 
 inline void Fun2() {}
 
 } // namespace a
+
+namespace b {
+void Fun3();
+} // namespace b

test/clang-move/Inputs/helper_decls_test.cpp

 static int HelperFun5() {
   int R = K5;
   return R;
 }
 
 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

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

 // CHECK-NEW-FUN2-H-NEXT: inline void Fun2() {}
 // CHECK-NEW-FUN2-H-NEXT: } // namespace a
 
 // CHECK-NEW-FUN2-CPP: #include "{{.*}}new_helper_decls_test.h"
 // CHECK-NEW-FUN2-CPP-SAME: {{[[:space:]]}}
 
 // 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
 // RUN: FileCheck -input-file=%T/used-helper-decls/helper_decls_test.cpp -allow-empty -check-prefix=CHECK-EMPTY %s
 
 
 // CHECK-NEW-H: namespace a {
 // CHECK-NEW-H-NEXT: class Class1 {

 // CHECK-NEW-CPP-NEXT: static int HelperFun5() {
 // CHECK-NEW-CPP-NEXT:   int R = K5;
 // CHECK-NEW-CPP-NEXT:   return R;
 // CHECK-NEW-CPP-NEXT: }
 // CHECK-NEW-CPP-SAME: {{[[:space:]]}}
 // 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: {{^}}{{$}}