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

(On Diff #82872)

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 ↗

(On Diff #82872)

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.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::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) {