[clang-move] Support template class.

[ioeric]

Nice!

First round of comments.

clang-move/ClangMove.cpp

414 ↗

(On Diff #77263)

Some comments here would be appreciated.

Also nit: no braces.

427 ↗

(On Diff #77263)

Same as above. Comments and braces.

test/clang-move/move-template-class.cpp

20 ↗

(On Diff #77263)

Not directly related, but we might want to start thinking about appropriate code formatting like spaces between decls. Code jamming together is not pretty...

[hokein]

Add comments.

[ioeric]

Lg with a few nits.

clang-move/ClangMove.cpp

417 ↗

(On Diff #77292)

erase(FTD ? FTD : CMD)

430 ↗

(On Diff #77292)

emplace_back(TC ? TC : CD, ...)

440 ↗

(On Diff #77292)

Similarly.

[hokein]

clang-move/ClangMove.cpp

417 ↗

(On Diff #77292)

We can't write the code like this way since the ternary operator (condition ? E1:E2) requires E1 and E2 has same type or they can convert to each other. In our case, the pointer types of FTD and CMD are different, and they can't convert to each other.

We could pass the compilation by writing the code like following way, but I'd keep the current way.

erase(FTD ? static_cast<NamedDecl*>(FTD) : static_cast<NamedDecl*>CMD);

[ioeric]

clang-move/ClangMove.cpp

417 ↗

(On Diff #77292)

nvm. you can keep it as it is.

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

void ClangMoveTool::registerMatchers(ast_matchers::MatchFinder *Finder) {
   Finder->addMatcher(MovedClass.bind("moved_class"), this);
   // Match moved class methods (static methods included) which are defined
   // outside moved class declaration.
   Finder->addMatcher(
       cxxMethodDecl(InOldFiles, ofOutermostEnclosingClass(*InMovedClassNames),
                     isDefinition())
           .bind("class_method"),
       this);
-
-  //============================================================================
-  // Matchers for old cc
-  //============================================================================
   // Match static member variable definition of the moved class.
   Finder->addMatcher(
-      varDecl(InMovedClass, InOldCC, isDefinition(), isStaticDataMember())
+      varDecl(InMovedClass, InOldFiles, isDefinition(), isStaticDataMember())
           .bind("class_static_var_decl"),
       this);
 
+  //============================================================================
+  // Matchers for old cc
+  //============================================================================
   auto InOldCCNamedNamespace =
       allOf(hasParent(namespaceDecl(unless(isAnonymous()))), InOldCC);
   // Matching using decls/type alias decls which are in named namespace. Those
   // in classes, functions and anonymous namespaces are covered in other
   // matchers.
   Finder->addMatcher(
       namedDecl(anyOf(usingDecl(InOldCCNamedNamespace),
                       usingDirectiveDecl(InOldCC, InOldCCNamedNamespace),
if (const auto *D =
     UnremovedDeclsInOldHeader.insert(D);
   } else if (const auto *CMD =
           Result.Nodes.getNodeAs<clang::CXXMethodDecl>("class_method")) {
     // Skip inline class methods. isInline() ast matcher doesn't ignore this
     // case.
     if (!CMD->isInlined()) {
       MovedDecls.emplace_back(CMD, &Result.Context->getSourceManager());
       RemovedDecls.push_back(MovedDecls.back());
+      // Get template class method from its method declaration as
+      // UnremovedDecls stores template class method.
+      if (const auto *FTD = CMD->getDescribedFunctionTemplate())
+        UnremovedDeclsInOldHeader.erase(FTD);
+      else
+        UnremovedDeclsInOldHeader.erase(CMD);
     }
   } else if (const auto *VD = Result.Nodes.getNodeAs<clang::VarDecl>(
                  "class_static_var_decl")) {
     MovedDecls.emplace_back(VD, &Result.Context->getSourceManager());
     RemovedDecls.push_back(MovedDecls.back());
-  } else if (const auto *class_decl =
+    UnremovedDeclsInOldHeader.erase(MovedDecls.back().Decl);
+  } else if (const auto *CD =
                  Result.Nodes.getNodeAs<clang::CXXRecordDecl>("moved_class")) {
-    MovedDecls.emplace_back(class_decl, &Result.Context->getSourceManager());
+    // Get class template from its class declaration as UnremovedDecls stores
+    // class template.
+    if (const auto * TC = CD->getDescribedClassTemplate())
+      MovedDecls.emplace_back(TC, &Result.Context->getSourceManager());
+    else
+      MovedDecls.emplace_back(CD, &Result.Context->getSourceManager());
     RemovedDecls.push_back(MovedDecls.back());
-    UnremovedDeclsInOldHeader.erase(class_decl);
+    UnremovedDeclsInOldHeader.erase(MovedDecls.back().Decl);
   } else if (const auto *FWD =
                  Result.Nodes.getNodeAs<clang::CXXRecordDecl>("fwd_decl")) {
     // Skip all forwad declarations which appear after moved class declaration.
     if (RemovedDecls.empty()) {
-      if (const auto *DCT = FWD->getDescribedClassTemplate()) {
+      if (const auto *DCT = FWD->getDescribedClassTemplate())
         MovedDecls.emplace_back(DCT, &Result.Context->getSourceManager());
-      } else {
+      else
         MovedDecls.emplace_back(FWD, &Result.Context->getSourceManager());
-      }
-    }
   } else if (const auto *ANS =
                  Result.Nodes.getNodeAs<clang::NamespaceDecl>("anonymous_ns")) {
     MovedDecls.emplace_back(ANS, &Result.Context->getSourceManager());
   } else if (const auto *ND =
                  Result.Nodes.getNodeAs<clang::NamedDecl>("static_decls")) {
     MovedDecls.emplace_back(ND, &Result.Context->getSourceManager());
   } else if (const auto *UD =
                  Result.Nodes.getNodeAs<clang::NamedDecl>("using_decl")) {

clang-tools-extra/trunk/test/clang-move/Inputs/template_class_test.h

+#ifndef TEMPLATE_CLASS_TEST_H // comment 1
+#define TEMPLATE_CLASS_TEST_H
+
+template <typename T>
+class A {
+ public:
+  void f();
+  void g();
+  template <typename U> void h();
+  template <typename U> void k();
+  static int b;
+  static int c;
+};
+
+template <typename T>
+void A<T>::f() {}
+
+template <typename T>
+template <typename U>
+void A<T>::h() {}
+
+template <typename T>
+int A<T>::b = 2;
+
+class B {
+ public:
+  void f();
+};
+
+#endif // TEMPLATE_CLASS_TEST_H

clang-tools-extra/trunk/test/clang-move/Inputs/template_class_test.cpp

+#include "template_class_test.h"
+
+template <typename T>
+void A<T>::g() {}
+
+template <typename T>
+template <typename U>
+void A<T>::k() {}
+
+template <typename T>
+int A<T>::c = 2;
+
+void B::f() {}

clang-tools-extra/trunk/test/clang-move/move-template-class.cpp

+// RUN: mkdir -p %T/move-template-class
+// RUN: cp %S/Inputs/template_class_test*  %T/move-template-class
+// RUN: cd %T/move-template-class
+// RUN: clang-move -names="A,B" -new_cc=%T/move-template-class/new_template_class_test.cpp -new_header=%T/move-template-class/new_template_class_test.h -old_cc=%T/move-template-class/template_class_test.cpp -old_header=../move-template-class/template_class_test.h %T/move-template-class/template_class_test.cpp --
+// RUN: FileCheck -input-file=%T/move-template-class/template_class_test.cpp -check-prefix=CHECK-OLD-TEST-EMPTY -allow-empty %s
+// RUN: FileCheck -input-file=%T/move-template-class/template_class_test.h -check-prefix=CHECK-OLD-TEST-EMPTY -allow-empty %s
+// RUN: FileCheck -input-file=%T/move-template-class/new_template_class_test.cpp -check-prefix=CHECK-NEW-TEST-CPP-CASE1 %s
+// RUN: FileCheck -input-file=%T/move-template-class/new_template_class_test.h -check-prefix=CHECK-NEW-TEST-H-CASE1 %s
+//
+// RUN: cp %S/Inputs/template_class_test*  %T/move-template-class
+// RUN: clang-move -names="A" -new_cc=%T/move-template-class/new_template_class_test.cpp -new_header=%T/move-template-class/new_template_class_test.h -old_cc=%T/move-template-class/template_class_test.cpp -old_header=../move-template-class/template_class_test.h %T/move-template-class/template_class_test.cpp --
+// RUN: FileCheck -input-file=%T/move-template-class/template_class_test.h -check-prefix=CHECK-OLD-TEST-H-CASE2 %s
+// RUN: FileCheck -input-file=%T/move-template-class/template_class_test.cpp -check-prefix=CHECK-OLD-TEST-CPP-CASE2 %s
+// RUN: FileCheck -input-file=%T/move-template-class/new_template_class_test.h -check-prefix=CHECK-NEW-TEST-H-CASE2 %s
+// RUN: FileCheck -input-file=%T/move-template-class/new_template_class_test.cpp -check-prefix=CHECK-NEW-TEST-CPP-CASE2 %s
+//
+//
+// CHECK-OLD-TEST-EMPTY: {{^}}{{$}}
+//
+// CHECK-NEW-TEST-H-CASE1: #ifndef TEMPLATE_CLASS_TEST_H // comment 1
+// CHECK-NEW-TEST-H-CASE1: #define TEMPLATE_CLASS_TEST_H
+// CHECK-NEW-TEST-H-CASE1: template <typename T>
+// CHECK-NEW-TEST-H-CASE1: class A {
+// CHECK-NEW-TEST-H-CASE1:  public:
+// CHECK-NEW-TEST-H-CASE1:   void f();
+// CHECK-NEW-TEST-H-CASE1:   void g();
+// CHECK-NEW-TEST-H-CASE1:   template <typename U> void h();
+// CHECK-NEW-TEST-H-CASE1:   template <typename U> void k();
+// CHECK-NEW-TEST-H-CASE1:   static int b;
+// CHECK-NEW-TEST-H-CASE1:   static int c;
+// CHECK-NEW-TEST-H-CASE1: };
+// CHECK-NEW-TEST-H-CASE1: template <typename T>
+// CHECK-NEW-TEST-H-CASE1: void A<T>::f() {}
+// CHECK-NEW-TEST-H-CASE1: template <typename T>
+// CHECK-NEW-TEST-H-CASE1: template <typename U>
+// CHECK-NEW-TEST-H-CASE1: void A<T>::h() {}
+// CHECK-NEW-TEST-H-CASE1: template <typename T>
+// CHECK-NEW-TEST-H-CASE1: int A<T>::b = 2;
+// CHECK-NEW-TEST-H-CASE1: class B {
+// CHECK-NEW-TEST-H-CASE1:  public:
+// CHECK-NEW-TEST-H-CASE1:   void f();
+// CHECK-NEW-TEST-H-CASE1: };
+// CHECK-NEW-TEST-H-CASE1: #endif // TEMPLATE_CLASS_TEST_H
+//
+// CHECK-NEW-TEST-CPP-CASE1: #include "{{.*}}new_template_class_test.h"
+// CHECK-NEW-TEST-CPP-CASE1: template <typename T>
+// CHECK-NEW-TEST-CPP-CASE1: void A<T>::g() {}
+// CHECK-NEW-TEST-CPP-CASE1: template <typename T>
+// CHECK-NEW-TEST-CPP-CASE1: template <typename U>
+// CHECK-NEW-TEST-CPP-CASE1: void A<T>::k() {}
+// CHECK-NEW-TEST-CPP-CASE1: template <typename T>
+// CHECK-NEW-TEST-CPP-CASE1: int A<T>::c = 2;
+// CHECK-NEW-TEST-CPP-CASE1: void B::f() {}
+//
+// CHECK-OLD-TEST-H-CASE2: #ifndef TEMPLATE_CLASS_TEST_H // comment 1
+// CHECK-OLD-TEST-H-CASE2: #define TEMPLATE_CLASS_TEST_H
+// CHECK-OLD-TEST-H-CASE2: class B {
+// CHECK-OLD-TEST-H-CASE2:  public:
+// CHECK-OLD-TEST-H-CASE2:   void f();
+// CHECK-OLD-TEST-H-CASE2: };
+// CHECK-OLD-TEST-H-CASE2: #endif // TEMPLATE_CLASS_TEST_H
+//
+// CHECK-OLD-TEST-CPP-CASE2: #include "template_class_test.h"
+// CHECK-OLD-TEST-CPP-CASE2:  void B::f() {}
+//
+// CHECK-NEW-TEST-H-CASE2: #ifndef {{.*}}NEW_TEMPLATE_CLASS_TEST_H
+// CHECK-NEW-TEST-H-CASE2: #define {{.*}}NEW_TEMPLATE_CLASS_TEST_H
+// CHECK-NEW-TEST-H-CASE2: template <typename T>
+// CHECK-NEW-TEST-H-CASE2: class A {
+// CHECK-NEW-TEST-H-CASE2:  public:
+// CHECK-NEW-TEST-H-CASE2:   void f();
+// CHECK-NEW-TEST-H-CASE2:   void g();
+// CHECK-NEW-TEST-H-CASE2:   template <typename U> void h();
+// CHECK-NEW-TEST-H-CASE2:   template <typename U> void k();
+// CHECK-NEW-TEST-H-CASE2:   static int b;
+// CHECK-NEW-TEST-H-CASE2:   static int c;
+// CHECK-NEW-TEST-H-CASE2: };
+// CHECK-NEW-TEST-H-CASE2: template <typename T> void A<T>::f() {}
+// CHECK-NEW-TEST-H-CASE2: template <typename T> template <typename U> void A<T>::h() {}
+// CHECK-NEW-TEST-H-CASE2: template <typename T> int A<T>::b = 2;
+// CHECK-NEW-TEST-H-CASE2: #endif // {{.*}}NEW_TEMPLATE_CLASS_TEST_H
+//
+// CHECK-NEW-TEST-CPP-CASE2: #include "{{.*}}new_template_class_test.h"
+// CHECK-NEW-TEST-CPP-CASE2: template <typename T> void A<T>::g() {}
+// CHECK-NEW-TEST-CPP-CASE2: template <typename T> template <typename U> void A<T>::k() {}
+// CHECK-NEW-TEST-CPP-CASE2: template <typename T> int A<T>::c = 2;