[clang-move] Support moving function.

[ioeric]

clang-move/ClangMove.cpp

414 ↗

(On Diff #77983)

s/ClassName/SymbolName/

417 ↗

(On Diff #77983)

I guess HasAnySymbolNames would be better when used in the matchers below.

test/clang-move/Inputs/function_test.h

1 ↗

(On Diff #77983)

Maybe add a test case where both classes and functions exist.

test/clang-move/move-function.cpp

9 ↗

(On Diff #77983)

Shouldn't there be empty lines around this line?

18 ↗

(On Diff #77983)

Why does this become one line after being moved?

Shouldn't it be:

template<typename T>
void h(T t) {}

[hokein]

Add one more test and rename variables.

test/clang-move/move-function.cpp

9 ↗

(On Diff #77983)

There should be. FileCheck will skip empty lines and try to find next matching line here. Have added some \n checks.

18 ↗

(On Diff #77983)

formatAndApplyReplacements does this for us ,and this is actually LLVM code style...

[ioeric]

Lg with two nits.

clang-move/ClangMove.cpp

145 ↗

(On Diff #78160)

Maybe just inline this variable since this is only used once.

146 ↗

(On Diff #78160)

I'd assert(FD) instead.

[hokein]

Fix nits.

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

if (const auto *FileEntry = SM.getFileEntryForID(SM.getFileID(HashLoc)))
                             FileEntry->getName(), FilenameRange, SM);
   }
 
 private:
   const SourceManager &SM;
   ClangMoveTool *const MoveTool;
 };
 
+class FunctionDeclarationMatch : public MatchFinder::MatchCallback {
+public:
+  explicit FunctionDeclarationMatch(ClangMoveTool *MoveTool)
+      : MoveTool(MoveTool) {}
+
+  void run(const MatchFinder::MatchResult &Result) override {
+    const auto *FD = Result.Nodes.getNodeAs<clang::FunctionDecl>("function");
+    assert(FD);
+    const clang::NamedDecl *D = FD;
+    if (const auto *FTD = FD->getDescribedFunctionTemplate())
+      D = FTD;
+    MoveTool->getMovedDecls().emplace_back(D,
+                                           &Result.Context->getSourceManager());
+    MoveTool->getUnremovedDeclsInOldHeader().erase(D);
+    MoveTool->getRemovedDecls().push_back(MoveTool->getMovedDecls().back());
+  }
+
+private:
+  ClangMoveTool *MoveTool;
+};
+
 class ClassDeclarationMatch : public MatchFinder::MatchCallback {
 public:
   explicit ClassDeclarationMatch(ClangMoveTool *MoveTool)
       : MoveTool(MoveTool) {}
   void run(const MatchFinder::MatchResult &Result) override {
     clang::SourceManager* SM = &Result.Context->getSourceManager();
     if (const auto *CMD =
             Result.Nodes.getNodeAs<clang::CXXMethodDecl>("class_method"))
ClangMoveTool::ClangMoveTool(
     : Spec(MoveSpec), FileToReplacements(FileToReplacements),
       OriginalRunningDirectory(OriginalRunningDirectory),
       FallbackStyle(FallbackStyle) {
   if (!Spec.NewHeader.empty())
     CCIncludes.push_back("#include \"" + Spec.NewHeader + "\"\n");
 }
 
 void ClangMoveTool::registerMatchers(ast_matchers::MatchFinder *Finder) {
-  Optional<ast_matchers::internal::Matcher<NamedDecl>> InMovedClassNames;
-  for (StringRef ClassName : Spec.Names) {
-    llvm::StringRef GlobalClassName = ClassName.trim().ltrim(':');
-    const auto HasName = hasName(("::" + GlobalClassName).str());
-    InMovedClassNames =
-        InMovedClassNames ? anyOf(*InMovedClassNames, HasName) : HasName;
+  Optional<ast_matchers::internal::Matcher<NamedDecl>> HasAnySymbolNames;
+  for (StringRef SymbolName: Spec.Names) {
+    llvm::StringRef GlobalSymbolName = SymbolName.trim().ltrim(':');
+    const auto HasName = hasName(("::" + GlobalSymbolName).str());
+    HasAnySymbolNames =
+        HasAnySymbolNames ? anyOf(*HasAnySymbolNames, HasName) : HasName;
   }
-  if (!InMovedClassNames) {
-    llvm::errs() << "No classes being moved.\n";
+  if (!HasAnySymbolNames) {
+    llvm::errs() << "No symbols being moved.\n";
     return;
   }
 
   auto InOldHeader = isExpansionInFile(makeAbsolutePath(Spec.OldHeader));
   auto InOldCC = isExpansionInFile(makeAbsolutePath(Spec.OldCC));
   auto InOldFiles = anyOf(InOldHeader, InOldCC);
   auto InMovedClass =
-      hasOutermostEnclosingClass(cxxRecordDecl(*InMovedClassNames));
+      hasOutermostEnclosingClass(cxxRecordDecl(*HasAnySymbolNames));
 
   auto ForwardDecls =
       cxxRecordDecl(unless(anyOf(isImplicit(), isDefinition())));
 
   //============================================================================
   // Matchers for old header
   //============================================================================
   // Match all top-level named declarations (e.g. function, variable, enum) in
void ClangMoveTool::registerMatchers(ast_matchers::MatchFinder *Finder) {
   //============================================================================
   // Matchers for old files, including old.h/old.cc
   //============================================================================
   // Create a MatchCallback for class declarations.
   MatchCallbacks.push_back(llvm::make_unique<ClassDeclarationMatch>(this));
   // Match moved class declarations.
   auto MovedClass =
       cxxRecordDecl(
-          InOldFiles, *InMovedClassNames, isDefinition(),
+          InOldFiles, *HasAnySymbolNames, isDefinition(),
           hasDeclContext(anyOf(namespaceDecl(), translationUnitDecl())))
           .bind("moved_class");
   Finder->addMatcher(MovedClass, MatchCallbacks.back().get());
   // Match moved class methods (static methods included) which are defined
   // outside moved class declaration.
   Finder->addMatcher(
-      cxxMethodDecl(InOldFiles, ofOutermostEnclosingClass(*InMovedClassNames),
+      cxxMethodDecl(InOldFiles, ofOutermostEnclosingClass(*HasAnySymbolNames),
                     isDefinition())
           .bind("class_method"),
       MatchCallbacks.back().get());
   // Match static member variable definition of the moved class.
   Finder->addMatcher(
       varDecl(InMovedClass, InOldFiles, isDefinition(), isStaticDataMember())
           .bind("class_static_var_decl"),
       MatchCallbacks.back().get());
 
+  MatchCallbacks.push_back(llvm::make_unique<FunctionDeclarationMatch>(this));
+  Finder->addMatcher(functionDecl(InOldFiles, *HasAnySymbolNames,
+                                  anyOf(hasDeclContext(namespaceDecl()),
+                                        hasDeclContext(translationUnitDecl())))
+                         .bind("function"),
+                     MatchCallbacks.back().get());
 }
 
 void ClangMoveTool::run(const ast_matchers::MatchFinder::MatchResult &Result) {
   if (const auto *D =
           Result.Nodes.getNodeAs<clang::NamedDecl>("decls_in_header")) {
     UnremovedDeclsInOldHeader.insert(D);
   } else if (const auto *FWD =
                  Result.Nodes.getNodeAs<clang::CXXRecordDecl>("fwd_decl")) {

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

+void f();
+
+inline int g() { return 0; }
+
+template<typename T>
+void h(T t) {}
+
+template<>
+void h(int t) {}
+
+class A {
+ public:
+  void f();
+};

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

+#include "function_test.h"
+
+void f() {}
+
+void A::f() {}

clang-tools-extra/trunk/test/clang-move/move-function.cpp

+// RUN: mkdir -p %T/move-function
+// RUN: cp %S/Inputs/function_test*  %T/move-function
+// RUN: cd %T/move-function
+// RUN: clang-move -names="g" -new_header=%T/move-function/new_function_test.h -old_header=../move-function/function_test.h %T/move-function/function_test.cpp --
+// RUN: FileCheck -input-file=%T/move-function/new_function_test.h -check-prefix=CHECK-NEW-TEST-H-CASE1 %s
+//
+// CHECK-NEW-TEST-H-CASE1: #ifndef {{.*}}NEW_FUNCTION_TEST_H
+// CHECK-NEW-TEST-H-CASE1: #define {{.*}}NEW_FUNCTION_TEST_H
+// CHECK-NEW-TEST-H-CASE1: {{[[:space:]]+}}
+// CHECK-NEW-TEST-H-CASE1: inline int g() { return 0; }
+// CHECK-NEW-TEST-H-CASE1: {{[[:space:]]+}}
+// CHECK-NEW-TEST-H-CASE1: #endif // {{.*}}NEW_FUNCTION_TEST_H
+//
+// RUN: cp %S/Inputs/function_test*  %T/move-function
+// RUN: clang-move -names="h" -new_header=%T/move-function/new_function_test.h -old_header=../move-function/function_test.h %T/move-function/function_test.cpp --
+// RUN: FileCheck -input-file=%T/move-function/new_function_test.h -check-prefix=CHECK-NEW-TEST-H-CASE2 %s
+//
+// CHECK-NEW-TEST-H-CASE2: #ifndef {{.*}}NEW_FUNCTION_TEST_H
+// CHECK-NEW-TEST-H-CASE2: #define {{.*}}NEW_FUNCTION_TEST_H
+// CHECK-NEW-TEST-H-CASE2: {{[[:space:]]+}}
+// CHECK-NEW-TEST-H-CASE2: template <typename T> void h(T t) {}
+// CHECK-NEW-TEST-H-CASE2: {{[[:space:]]+}}
+// CHECK-NEW-TEST-H-CASE2: template <> void h(int t) {}
+// CHECK-NEW-TEST-H-CASE2: {{[[:space:]]+}}
+// CHECK-NEW-TEST-H-CASE2: #endif // {{.*}}NEW_FUNCTION_TEST_H
+//
+// RUN: cp %S/Inputs/function_test*  %T/move-function
+// RUN: clang-move -names="f" -new_header=%T/move-function/new_function_test.h -new_cc=%T/move-function/new_function_test.cpp -old_header=../move-function/function_test.h -old_cc=../move-function/function_test.cpp %T/move-function/function_test.cpp --
+// RUN: FileCheck -input-file=%T/move-function/new_function_test.h -check-prefix=CHECK-NEW-TEST-H-CASE3 %s
+// RUN: FileCheck -input-file=%T/move-function/new_function_test.cpp -check-prefix=CHECK-NEW-TEST-CPP-CASE3 %s
+//
+// CHECK-NEW-TEST-H-CASE3: #ifndef {{.*}}NEW_FUNCTION_TEST_H
+// CHECK-NEW-TEST-H-CASE3: #define {{.*}}NEW_FUNCTION_TEST_H
+// CHECK-NEW-TEST-H-CASE3: {{[[:space:]]+}}
+// CHECK-NEW-TEST-H-CASE3: void f();
+// CHECK-NEW-TEST-H-CASE3: {{[[:space:]]+}}
+// CHECK-NEW-TEST-H-CASE3: #endif // {{.*}}NEW_FUNCTION_TEST_H
+// CHECK-NEW-TEST-CPP-CASE3: #include "{{.*}}new_function_test.h"
+// CHECK-NEW-TEST-CPP-CASE3: {{[[:space:]]+}}
+// CHECK-NEW-TEST-CPP-CASE3: void f() {}
+//
+// RUN: cp %S/Inputs/function_test*  %T/move-function
+// RUN: clang-move -names="A::f" -new_header=%T/move-function/new_function_test.h -new_cc=%T/move-function/new_function_test.cpp -old_header=../move-function/function_test.h -old_cc=../move-function/function_test.cpp %T/move-function/function_test.cpp -dump_result -- | FileCheck %s -check-prefix=CHECK-EMPTY
+//
+// CHECK-EMPTY: [{{[[:space:]]*}}]
+//
+// RUN: cp %S/Inputs/function_test*  %T/move-function
+// RUN: clang-move -names="f,A" -new_header=%T/move-function/new_function_test.h -new_cc=%T/move-function/new_function_test.cpp -old_header=../move-function/function_test.h -old_cc=../move-function/function_test.cpp %T/move-function/function_test.cpp --
+// RUN: FileCheck -input-file=%T/move-function/new_function_test.h -check-prefix=CHECK-NEW-TEST-H-CASE4 %s
+// RUN: FileCheck -input-file=%T/move-function/new_function_test.cpp -check-prefix=CHECK-NEW-TEST-CPP-CASE4 %s
+
+// CHECK-NEW-TEST-H-CASE4: #ifndef {{.*}}NEW_FUNCTION_TEST_H
+// CHECK-NEW-TEST-H-CASE4: #define {{.*}}NEW_FUNCTION_TEST_H
+// CHECK-NEW-TEST-H-CASE4: {{[[:space:]]+}}
+// CHECK-NEW-TEST-H-CASE4: void f();
+// CHECK-NEW-TEST-H-CASE4: {{[[:space:]]+}}
+// CHECK-NEW-TEST-H-CASE4: class A {
+// CHECK-NEW-TEST-H-CASE4: public:
+// CHECK-NEW-TEST-H-CASE4:   void f();
+// CHECK-NEW-TEST-H-CASE4: };
+// CHECK-NEW-TEST-H-CASE4: {{[[:space:]]+}}
+// CHECK-NEW-TEST-H-CASE4: #endif // {{.*}}NEW_FUNCTION_TEST_H
+// CHECK-NEW-TEST-CPP-CASE4: #include "{{.*}}new_function_test.h"
+// CHECK-NEW-TEST-CPP-CASE4: {{[[:space:]]+}}
+// CHECK-NEW-TEST-CPP-CASE4: void f() {}
+// CHECK-NEW-TEST-CPP-CASE4: {{[[:space:]]+}}
+// CHECK-NEW-TEST-CPP-CASE4: void A::f() {}