[clang-move] Abstract a ClassMather for matching class declarations.

[hokein]

Summary

No functionality change.

This is a refactoring patch, which makes the code more readable and easy
to extend it to support more types.

[ioeric]

clang-move/ClangMove.cpp

144

It'd be more readable if you pull each case below into a helper function.

422

push_back and then use back() would save you a variable.

444

I'd first move push_back to the top, and then use MatchCallbacksHolder.back().

clang-move/ClangMove.h

104

Simply MatchCallbacks would be a better name IMO.

[hokein]

Address comments.

clang-move/ClangMove.cpp

144

hmm, I'd prefer current way. Because I don't see significant advantage by pulling each case into a helper function, since the code for each condition is used only once.

[ioeric]

clang-move/ClangMove.cpp

139

Why is this called a Matcher?

144

Generally, we prefer short functions to long functions.

[hokein]

Split to small functions.

clang-move/ClangMove.cpp

139

Rename it to a better name "Match".

[ioeric]

Lg with a few comments.

(There are two empty comments that can't be deleted due to a phabricator bugs...)

clang-move/ClangMove.cpp

151

Not sure why you don't do type check in run(...). The following structure looks a bit weird.

if (...) {
  ...
  return true;
}
return false;

414

Maybe put all addMatchers that bind this before those that bind to ClassDeclarationMatch.

426

.

474

-

[hokein]

Address remaining comments.

clang-move/ClangMove.h

 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_MOVE_CLANGMOVE_H
 
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Frontend/FrontendAction.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include <map>
+#include <memory>
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace move {
 
 // FIXME: Make it support more types, e.g. function definitions.
 // Currently only support moving class definition.
public:
   /// \param IncludeFilenameRange The source range for the written file name in #include
   ///  (i.e. "old.h" for #include "old.h") in old.cc.
   /// \param SM The SourceManager.
   void addIncludes(llvm::StringRef IncludeHeader, bool IsAngled,
                    llvm::StringRef SearchPath, llvm::StringRef FileName,
                    clang::CharSourceRange IncludeFilenameRange,
                    const SourceManager &SM);
 
+  std::vector<MovedDecl> &getMovedDecls() { return MovedDecls; }
+
+  std::vector<MovedDecl> &getRemovedDecls() { return RemovedDecls; }
+
+  llvm::SmallPtrSet<const NamedDecl *, 8> &getUnremovedDeclsInOldHeader() {
+    return UnremovedDeclsInOldHeader;
+  }
+
 private:
   // Make the Path absolute using the OrignalRunningDirectory if the Path is not
   // an absolute path. An empty Path will result in an empty string.
   std::string makeAbsolutePath(StringRef Path);
 
   void removeClassDefinitionInOldFiles();
   void moveClassDefinitionToNewFiles();
   void moveAll(SourceManager& SM, StringRef OldFile, StringRef NewFile);
 
   MoveDefinitionSpec Spec;
+  // Stores all MatchCallbacks created by this tool.
+  std::vector<std::unique_ptr<ast_matchers::MatchFinder::MatchCallback>>
+      MatchCallbacks;
   // The Key is file path, value is the replacements being applied to the file.
   std::map<std::string, tooling::Replacements> &FileToReplacements;
   // All declarations (the class decl being moved, forward decls) that need to
   // be moved/copy to the new files, saving in an AST-visited order.
   std::vector<MovedDecl> MovedDecls;
   // The declarations that needs to be removed in old.cc/h.
   std::vector<MovedDecl> RemovedDecls;
   // The #includes in old_header.h.

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 ClassDeclarationMatch : public MatchFinder::MatchCallback {
+public:
+  explicit ClassDeclarationMatch(ClangMoveTool *MoveTool)
+      : MoveTool(MoveTool) {}
+  void run(const MatchFinder::MatchResult &Result) override {
+    if (MatchClassMethod(Result)) return;
+    if (MatchClassStaticVariable(Result)) return;
+    if (MatchClassDeclaration(Result)) return;
+  }
+
+private:
+  bool MatchClassMethod(const MatchFinder::MatchResult &Result) {
+    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()) {
+        MoveTool->getMovedDecls().emplace_back(
+            CMD, &Result.Context->getSourceManager());
+        MoveTool->getRemovedDecls().push_back(MoveTool->getMovedDecls().back());
+        // Get template class method from its method declaration as
+        // UnremovedDecls stores template class method.
+        if (const auto *FTD = CMD->getDescribedFunctionTemplate()) {
+          MoveTool->getUnremovedDeclsInOldHeader().erase(FTD);
+        } else {
+          MoveTool->getUnremovedDeclsInOldHeader().erase(CMD);
+        }
+      }
+      return true;
+    }
+    return false;
+  }
+
+  bool MatchClassStaticVariable(const MatchFinder::MatchResult &Result) {
+   if (const auto *VD = Result.Nodes.getNodeAs<clang::VarDecl>(
+                   "class_static_var_decl")) {
+      MoveTool->getMovedDecls().emplace_back(
+          VD, &Result.Context->getSourceManager());
+      MoveTool->getRemovedDecls().push_back(MoveTool->getMovedDecls().back());
+      MoveTool->getUnremovedDeclsInOldHeader().erase(VD);
+      return true;
+    }
+    return false;
+  }
+
+  bool MatchClassDeclaration(const MatchFinder::MatchResult &Result) {
+    if (const auto *CD = Result.Nodes.getNodeAs<clang::CXXRecordDecl>(
+                   "moved_class")) {
+      // Get class template from its class declaration as UnremovedDecls stores
+      // class template.
+      if (const auto *TC = CD->getDescribedClassTemplate()) {
+        MoveTool->getMovedDecls().emplace_back(
+            TC, &Result.Context->getSourceManager());
+      } else {
+        MoveTool->getMovedDecls().emplace_back(
+            CD, &Result.Context->getSourceManager());
+      }
+      MoveTool->getRemovedDecls().push_back(MoveTool->getMovedDecls().back());
+      MoveTool->getUnremovedDeclsInOldHeader().erase(
+          MoveTool->getMovedDecls().back().Decl);
+      return true;
+    }
+    return false;
+  }
+  ClangMoveTool *MoveTool;
+};
+
 // Expand to get the end location of the line where the EndLoc of the given
 // Decl.
 SourceLocation
 getLocForEndOfDecl(const clang::Decl *D, const SourceManager *SM,
                    const LangOptions &LangOpts = clang::LangOptions()) {
   std::pair<FileID, unsigned> LocInfo = SM->getDecomposedLoc(D->getLocEnd());
   // Try to load the file buffer.
   bool InvalidTemp = false;
void ClangMoveTool::registerMatchers(ast_matchers::MatchFinder *Finder) {
   // The old header which contains only one declaration being moved and forward
   // declarations is considered to be moved totally.
   auto AllDeclsInHeader = namedDecl(
       unless(ForwardDecls), unless(namespaceDecl()),
       unless(usingDirectiveDecl()), // using namespace decl.
       unless(classTemplateDecl(has(ForwardDecls))), // template forward decl.
       InOldHeader,
       hasParent(decl(anyOf(namespaceDecl(), translationUnitDecl()))));
   Finder->addMatcher(AllDeclsInHeader.bind("decls_in_header"), this);
   // Match forward declarations in old header.
   Finder->addMatcher(namedDecl(ForwardDecls, InOldHeader).bind("fwd_decl"),
                      this);
 
   //============================================================================
   // 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(
+  auto MovedClass =
+      cxxRecordDecl(
           InOldFiles, *InMovedClassNames, isDefinition(),
-      hasDeclContext(anyOf(namespaceDecl(), translationUnitDecl())));
-  Finder->addMatcher(MovedClass.bind("moved_class"), this);
+          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),
                     isDefinition())
           .bind("class_method"),
-      this);
+      MatchCallbacks.back().get());
   // Match static member variable definition of the moved class.
   Finder->addMatcher(
       varDecl(InMovedClass, InOldFiles, isDefinition(), isStaticDataMember())
           .bind("class_static_var_decl"),
-      this);
+      MatchCallbacks.back().get());
 
   //============================================================================
   // 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(
auto IsOldCCStaticDefinition =
       allOf(isDefinition(), unless(InMovedClass), InOldCCNamedNamespace,
             isStaticStorageClass());
   Finder->addMatcher(namedDecl(anyOf(functionDecl(IsOldCCStaticDefinition),
                                      varDecl(IsOldCCStaticDefinition)))
                          .bind("static_decls"),
                      this);
 }
 
 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 *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());
-    UnremovedDeclsInOldHeader.erase(MovedDecls.back().Decl);
-  } else if (const auto *CD =
-                 Result.Nodes.getNodeAs<clang::CXXRecordDecl>("moved_class")) {
-    // 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(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())
         MovedDecls.emplace_back(DCT, &Result.Context->getSourceManager());
       else
         MovedDecls.emplace_back(FWD, &Result.Context->getSourceManager());