[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 ↗

(On Diff #77517)

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

403 ↗

(On Diff #77517)

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

424 ↗

(On Diff #77517)

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

clang-move/ClangMove.h

104 ↗

(On Diff #77517)

Simply MatchCallbacks would be a better name IMO.

[hokein]

Address comments.

clang-move/ClangMove.cpp

144 ↗

(On Diff #77517)

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 ↗

(On Diff #77521)

Why is this called a Matcher?

144 ↗

(On Diff #77517)

Generally, we prefer short functions to long functions.

[hokein]

Split to small functions.

clang-move/ClangMove.cpp

139 ↗

(On Diff #77521)

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 ↗

(On Diff #77788)

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

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

414 ↗

(On Diff #77788)

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

407 ↗

(On Diff #77521)

.

455 ↗

(On Diff #77521)

-

[hokein]

Address remaining comments.

clang-tools-extra/trunk/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-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 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"))
+      MatchClassMethod(CMD, SM);
+    else if (const auto *VD = Result.Nodes.getNodeAs<clang::VarDecl>(
+                   "class_static_var_decl"))
+      MatchClassStaticVariable(VD, SM);
+    else if (const auto *CD = Result.Nodes.getNodeAs<clang::CXXRecordDecl>(
+                   "moved_class"))
+      MatchClassDeclaration(CD, SM);
+  }
+
+private:
+  void MatchClassMethod(const clang::CXXMethodDecl* CMD,
+                        clang::SourceManager* SM) {
+    // Skip inline class methods. isInline() ast matcher doesn't ignore this
+    // case.
+    if (!CMD->isInlined()) {
+      MoveTool->getMovedDecls().emplace_back(CMD, SM);
+      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);
+    }
+  }
+
+  void MatchClassStaticVariable(const clang::NamedDecl *VD,
+                                clang::SourceManager* SM) {
+    MoveTool->getMovedDecls().emplace_back(VD, SM);
+    MoveTool->getRemovedDecls().push_back(MoveTool->getMovedDecls().back());
+    MoveTool->getUnremovedDeclsInOldHeader().erase(VD);
+  }
+
+  void MatchClassDeclaration(const clang::CXXRecordDecl *CD,
+                             clang::SourceManager* SM) {
+    // Get class template from its class declaration as UnremovedDecls stores
+    // class template.
+    if (const auto *TC = CD->getDescribedClassTemplate())
+      MoveTool->getMovedDecls().emplace_back(TC, SM);
+    else
+      MoveTool->getMovedDecls().emplace_back(CD, SM);
+    MoveTool->getRemovedDecls().push_back(MoveTool->getMovedDecls().back());
+    MoveTool->getUnremovedDeclsInOldHeader().erase(
+        MoveTool->getMovedDecls().back().Decl);
+  }
+
+  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;
auto AllDeclsInHeader = namedDecl(
       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
-  //============================================================================
-  // Match moved class declarations.
-  auto MovedClass = cxxRecordDecl(
-      InOldFiles, *InMovedClassNames, isDefinition(),
-      hasDeclContext(anyOf(namespaceDecl(), translationUnitDecl())));
-  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);
-  // Match static member variable definition of the moved class.
-  Finder->addMatcher(
-      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(
void ClangMoveTool::registerMatchers(ast_matchers::MatchFinder *Finder) {
   // namespaces.
   auto IsOldCCStaticDefinition =
       allOf(isDefinition(), unless(InMovedClass), InOldCCNamedNamespace,
             isStaticStorageClass());
   Finder->addMatcher(namedDecl(anyOf(functionDecl(IsOldCCStaticDefinition),
                                      varDecl(IsOldCCStaticDefinition)))
                          .bind("static_decls"),
                      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(
+          InOldFiles, *InMovedClassNames, 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),
+                    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());
+
 }
 
 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());