[clang-move] Support moving enum declarations.

[ioeric]

Do we consider enum helpers?

clang-move/ClangMove.cpp

192

These 3 lines seen to be repeated. Maybe pull them out as a MoveTool method?

[hokein]

Address review comment.

[hokein]

In D28228#633863, @ioeric wrote:

Do we consider enum helpers?

This patch excludes enum helpers, only considers the enum declarations which are defined in old.h.
Ideally, we should support enum helpers which are defined in old.cc, but that's another topic.

clang-move/ClangMove.cpp

192

Done. I made a function for these repeated lines, but it seems hard to figure out a pretty name for it.

[ioeric]

In D28228#633925, @hokein wrote:

In D28228#633863, @ioeric wrote:

Do we consider enum helpers?

This patch excludes enum helpers, only considers the enum declarations which are defined in old.h.
Ideally, we should support enum helpers which are defined in old.cc, but that's another topic.

I mean, if a enum helper used in say only old cc, will it be moved? The question is related to D27673. You don't need to support enum helper now, but it would be nice to drop a comment somewhere.

clang-move/ClangMove.h

140

Maybe just addMovedDecl? move indicates move and delete IMO. But you might need to explain what's really going on in the comment.

[hokein]

Refine function name and add a comment for enum helpers.

[hokein]

In D28228#633930, @ioeric wrote:

I mean, if a enum helper used in say only old cc, will it be moved? The question is related to D27673. You don't need to support enum helper now, but it would be nice to drop a comment somewhere.

Done. Add a comment about this. Enum helpers will not moved whether they are used or not.

clang-move/ClangMove.h

140

Instead of making an interface in the class, I put this function as a helper function in .cc, and named it as MoveDeclFromOldFileToNewFile. Hope this will make it clearer.

[ioeric]

Lg

clang-move/ClangMove.cpp

456

Remove this?

[hokein]

Remove forgot-deleted code.

[hokein]

Not change in ClangMove.h

clang-move/ClangMove.h

void addIncludes(llvm::StringRef IncludeHeader, bool IsAngled,
                    llvm::StringRef SearchPath, llvm::StringRef FileName,
                    clang::CharSourceRange IncludeFilenameRange,
                    const SourceManager &SM);
 
   std::vector<const NamedDecl *> &getMovedDecls() { return MovedDecls; }
 
   /// Add declarations being removed from old.h/cc. For each declarations, the
   /// method also records the mapping relationship between the corresponding
   /// FilePath and its FileID.
   void addRemovedDecl(const NamedDecl *Decl);
 
   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 removeDeclsInOldFiles();
   void moveDeclsToNewFiles();
   void moveAll(SourceManager& SM, StringRef OldFile, StringRef NewFile);
 
+
   // Stores all MatchCallbacks created by this tool.
   std::vector<std::unique_ptr<ast_matchers::MatchFinder::MatchCallback>>
       MatchCallbacks;
   // Store all potential declarations (decls being moved, forward decls) that
   // might need to move to new.h/cc. It includes all helper declarations
   // (include unused ones) by default. The unused ones will be filtered out in
   // the last stage. Saving in an AST-visited order.
   std::vector<const NamedDecl *> MovedDecls;

clang-move/ClangMove.cpp

if (const auto *FileEntry = SM.getFileEntryForID(SM.getFileID(HashLoc)))
                             FileEntry->getName(), FilenameRange, SM);
   }
 
 private:
   const SourceManager &SM;
   ClangMoveTool *const MoveTool;
 };
 
+/// Add a declatration being moved to new.h/cc. Note that the declaration will
+/// also be deleted in old.h/cc.
+void MoveDeclFromOldFileToNewFile(ClangMoveTool *MoveTool, const NamedDecl *D) {
+  MoveTool->getMovedDecls().push_back(D);
+  MoveTool->addRemovedDecl(D);
+  MoveTool->getUnremovedDeclsInOldHeader().erase(D);
+}
+
 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().push_back(D);
-    MoveTool->getUnremovedDeclsInOldHeader().erase(D);
-    MoveTool->addRemovedDecl(D);
+    MoveDeclFromOldFileToNewFile(MoveTool, D);
+  }
+
+private:
+  ClangMoveTool *MoveTool;
+};
+
+class EnumDeclarationMatch : public MatchFinder::MatchCallback {
+public:
+  explicit EnumDeclarationMatch(ClangMoveTool *MoveTool)
+      : MoveTool(MoveTool) {}
+
+  void run(const MatchFinder::MatchResult &Result) override {
+    const auto *ED = Result.Nodes.getNodeAs<clang::EnumDecl>("enum");
+    assert(ED);
+    MoveDeclFromOldFileToNewFile(MoveTool, ED);
   }
 
 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"))
       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);
   }
 
if (!CMD->isInlined()) {
         MoveTool->getUnremovedDeclsInOldHeader().erase(FTD);
       else
         MoveTool->getUnremovedDeclsInOldHeader().erase(CMD);
     }
   }
 
   void MatchClassStaticVariable(const clang::NamedDecl *VD,
                                 clang::SourceManager* SM) {
-    MoveTool->getMovedDecls().push_back(VD);
-    MoveTool->addRemovedDecl(VD);
-    MoveTool->getUnremovedDeclsInOldHeader().erase(VD);
+    MoveDeclFromOldFileToNewFile(MoveTool, 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().push_back(TC);

 
 ClangMoveTool::ClangMoveTool(ClangMoveContext *const Context,
                              DeclarationReporter *const Reporter)
     : Context(Context), Reporter(Reporter) {
   if (!Context->Spec.NewHeader.empty())
     CCIncludes.push_back("#include \"" + Context->Spec.NewHeader + "\"\n");
 }
 
+void ClangMoveTool::addMovedAndDeletedDecl(const NamedDecl *D) {
+  MovedDecls.push_back(D);
+  addRemovedDecl(D);
+  UnremovedDeclsInOldHeader.erase(D);
+}
+
 void ClangMoveTool::addRemovedDecl(const NamedDecl *Decl) {
   const auto &SM = Decl->getASTContext().getSourceManager();
   auto Loc = Decl->getLocation();
   StringRef FilePath = SM.getFilename(Loc);
   FilePathToFileID[FilePath] = SM.getFileID(Loc);
   RemovedDecls.push_back(Decl);
 }
 
 void ClangMoveTool::registerMatchers(ast_matchers::MatchFinder *Finder) {
   auto InOldHeader =
       isExpansionInFile(makeAbsolutePath(Context->Spec.OldHeader));
   auto InOldCC = isExpansionInFile(makeAbsolutePath(Context->Spec.OldCC));
   auto InOldFiles = anyOf(InOldHeader, InOldCC);
   auto ForwardDecls =
       cxxRecordDecl(unless(anyOf(isImplicit(), isDefinition())));
+  auto TopLevelDecl =
+      hasDeclContext(anyOf(namespaceDecl(), translationUnitDecl()));
 
   //============================================================================
   // Matchers for old header
   //============================================================================
   // Match all top-level named declarations (e.g. function, variable, enum) in
   // old header, exclude forward class declarations and namespace declarations.
   //
   // We consider declarations inside a class belongs to the class. So these
Finder->addMatcher(
       &RGBuilder);
 
   //============================================================================
   // 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, *HasAnySymbolNames, isDefinition(),
-          hasDeclContext(anyOf(namespaceDecl(), translationUnitDecl())))
+  auto MovedClass = cxxRecordDecl(InOldFiles, *HasAnySymbolNames,
+                                  isDefinition(), TopLevelDecl)
                         .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(*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())))
+  Finder->addMatcher(functionDecl(InOldFiles, *HasAnySymbolNames, TopLevelDecl)
                          .bind("function"),
                      MatchCallbacks.back().get());
+
+  // Match enum definition in old.h. Enum helpers (which are definied in old.cc)
+  // will not be moved for now no matter whether they are used or not.
+  MatchCallbacks.push_back(llvm::make_unique<EnumDeclarationMatch>(this));
+  Finder->addMatcher(
+      enumDecl(InOldHeader, *HasAnySymbolNames, isDefinition(), TopLevelDecl)
+          .bind("enum"),
+      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")) {
void ClangMoveTool::onEndOfTranslationUnit() {
   // we always move old files to new files when all symbols produced from
   // dump_decls are moved.
   auto IsSupportedKind = [](const clang::NamedDecl *Decl) {
     switch (Decl->getKind()) {
     case Decl::Kind::Function:
     case Decl::Kind::FunctionTemplate:
     case Decl::Kind::ClassTemplate:
     case Decl::Kind::CXXRecord:
+    case Decl::Kind::Enum:
       return true;
     default:
       return false;
     }
   };
   if (std::none_of(UnremovedDeclsInOldHeader.begin(),
                    UnremovedDeclsInOldHeader.end(), IsSupportedKind) &&
       !Context->Spec.OldHeader.empty()) {

test/clang-move/Inputs/enum.h

+namespace a {
+enum E1 { Green, Red };
+
+enum class E2 { Yellow };
+
+class C {
+  enum E3 { Blue };
+};
+} // namespace a

test/clang-move/move-enum-decl.cpp

+// RUN: mkdir -p %T/move-enum
+// RUN: cp %S/Inputs/enum.h  %T/move-enum/enum.h
+// RUN: echo '#include "enum.h"' > %T/move-enum/enum.cpp
+// RUN: cd %T/move-enum
+//
+// -----------------------------------------------------------------------------
+// Test moving enum declarations.
+// -----------------------------------------------------------------------------
+// RUN: clang-move -names="a::E1" -new_cc=%T/move-enum/new_test.cpp -new_header=%T/move-enum/new_test.h -old_cc=%T/move-enum/enum.cpp -old_header=%T/move-enum/enum.h %T/move-enum/enum.cpp -- -std=c++11
+// RUN: FileCheck -input-file=%T/move-enum/new_test.h -check-prefix=CHECK-NEW-TEST-H-CASE1 %s
+// RUN: FileCheck -input-file=%T/move-enum/enum.h -check-prefix=CHECK-OLD-TEST-H-CASE1 %s
+//
+// CHECK-NEW-TEST-H-CASE1: namespace a {
+// CHECK-NEW-TEST-H-CASE1-NEXT: enum E1 { Green, Red };
+// CHECK-NEW-TEST-H-CASE1-NEXT: }
+
+// CHECK-OLD-TEST-H-CASE1-NOT: enum E1 { Green, Red };
+
+
+// -----------------------------------------------------------------------------
+// Test moving scoped enum declarations.
+// -----------------------------------------------------------------------------
+// RUN: cp %S/Inputs/enum.h  %T/move-enum/enum.h
+// RUN: echo '#include "enum.h"' > %T/move-enum/enum.cpp
+// RUN: clang-move -names="a::E2" -new_cc=%T/move-enum/new_test.cpp -new_header=%T/move-enum/new_test.h -old_cc=%T/move-enum/enum.cpp -old_header=%T/move-enum/enum.h %T/move-enum/enum.cpp -- -std=c++11
+// RUN: FileCheck -input-file=%T/move-enum/new_test.h -check-prefix=CHECK-NEW-TEST-H-CASE2 %s
+// RUN: FileCheck -input-file=%T/move-enum/enum.h -check-prefix=CHECK-OLD-TEST-H-CASE2 %s
+
+// CHECK-NEW-TEST-H-CASE2: namespace a {
+// CHECK-NEW-TEST-H-CASE2-NEXT: enum class E2 { Yellow };
+// CHECK-NEW-TEST-H-CASE2-NEXT: }
+
+// CHECK-OLD-TEST-H-CASE2-NOT: enum class E2 { Yellow };
+
+
+// -----------------------------------------------------------------------------
+// Test not moving class-insided enum declarations.
+// -----------------------------------------------------------------------------
+// RUN: cp %S/Inputs/enum.h  %T/move-enum/enum.h
+// RUN: echo '#include "enum.h"' > %T/move-enum/enum.cpp
+// RUN: clang-move -names="a::C::E3" -new_cc=%T/move-enum/new_test.cpp -new_header=%T/move-enum/new_test.h -old_cc=%T/move-enum/enum.cpp -old_header=%T/move-enum/enum.h %T/move-enum/enum.cpp -- -std=c++11
+// RUN: FileCheck -input-file=%T/move-enum/new_test.h -allow-empty -check-prefix=CHECK-EMPTY %s
+
+// CHECK-EMPTY: {{^}}{{$}}

unittests/clang-move/ClangMoveTests.cpp

TEST(ClangMove, DontMoveAll) {
   }
 }
 
 TEST(ClangMove, IgnoreUnsupportedKindsAndMoveAll) {
   const char Code[] = "#include \"foo.h\"\nint A::f() { return 0; }";
   std::vector<std::string> TestHeaders = {
       "typedef int Int;\nclass A {\npublic:\n  int f();\n};\n",
       "using Int = int;\nclass A {\npublic:\n  int f();\n};\n",
-      "enum Color { RED };\nclass A {\npublic:\n  int f();\n};\n",
   };
   move::MoveDefinitionSpec Spec;
   Spec.Names.push_back("A");
   Spec.OldHeader = "foo.h";
   Spec.OldCC = "foo.cc";
   Spec.NewHeader = "new_foo.h";
   Spec.NewCC = "new_foo.cc";
   for (const auto &Header : TestHeaders) {