[clang-move] Support moving multiple classes in one run.

[ioeric]

clang-move/ClangMove.cpp

295 ↗

(On Diff #73756)

Why create this lambda if it's only used once? I think you can save a few lines by just inlining this.

303 ↗

(On Diff #73756)

I'd also allow whitespaces around separators, e.g. a::b::X, a::b::Y. Maybe also trim() the name.

304 ↗

(On Diff #73756)

Does this work?

InMovedClassNames = InMovedClassNames ? ... : ...

316 ↗

(On Diff #73756)

What would happen if InMovedClassNames == false?

clang-move/ClangMove.h

40 ↗

(On Diff #73756)

I'd use commas as separator. Eyeballs can easily mix ";" with ":".

clang-move/tool/ClangMoveMain.cpp

41 ↗

(On Diff #73756)

Same here if semi->comma.

And I'd name this Names and names

test/clang-move/Inputs/multiple_class_test.cpp

15 ↗

(On Diff #73756)

Can you add one more moved class that is also in namespace c?

[hokein]

Address review comments.

clang-move/ClangMove.cpp

316 ↗

(On Diff #73756)

"InMovedClassNames" should not be false in the matcher. Added an assert.

[ioeric]

clang-move/ClangMove.cpp

299 ↗

(On Diff #73819)

The comment is trivial.

302 ↗

(On Diff #73819)

Can we use trim() with default argument?

316 ↗

(On Diff #73756)

I think you should instead exit early if ClassNames.empty() or assert not empty.

test/clang-move/Inputs/multiple_class_test.cpp

19 ↗

(On Diff #73819)

What I wanted to see is another *moved* class because there was no test case where multiple classes are moved into the same namespace.

[hokein]

Update.

[ioeric]

Lg. Thanks!

[ioeric]

clang-move/ClangMove.cpp

291–297 ↗

(On Diff #73888)

Spamming.

Test phabricator.

316 ↗

(On Diff #73756)

Test nested comments.

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

struct MovedDecl {
     const clang::NamedDecl *Decl = nullptr;
     clang::SourceManager *SM = nullptr;
     MovedDecl() = default;
     MovedDecl(const clang::NamedDecl *Decl, clang::SourceManager *SM)
         : Decl(Decl), SM(SM) {}
   };
 
   struct MoveDefinitionSpec {
-    // A fully qualified name, e.g. "X", "a::X".
-    std::string Name;
+    // A comma-separated list of fully qualified names, e.g. "Foo",
+    // "a::Foo, b::Foo".
+    std::string Names;
     // The file path of old header, can be relative path and absolute path.
     std::string OldHeader;
     // The file path of old cc, can be relative path and absolute path.
     std::string OldCC;
     // The file path of new header, can be relative path and absolute path.
     std::string NewHeader;
     // The file path of new cc, can be relative path and absolute path.
     std::string NewCC;

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

 
 ClangMoveTool::ClangMoveTool(
     const MoveDefinitionSpec &MoveSpec,
     std::map<std::string, tooling::Replacements> &FileToReplacements,
     llvm::StringRef OriginalRunningDirectory, llvm::StringRef FallbackStyle)
     : Spec(MoveSpec), FileToReplacements(FileToReplacements),
       OriginalRunningDirectory(OriginalRunningDirectory),
       FallbackStyle(FallbackStyle) {
-  Spec.Name = llvm::StringRef(Spec.Name).ltrim(':');
   if (!Spec.NewHeader.empty())
     CCIncludes.push_back("#include \"" + Spec.NewHeader + "\"\n");
 }
 
 void ClangMoveTool::registerMatchers(ast_matchers::MatchFinder *Finder) {
-  std::string FullyQualifiedName = "::" + Spec.Name;
+  SmallVector<StringRef, 4> ClassNames;
+  llvm::StringRef(Spec.Names).split(ClassNames, ',');
+  Optional<ast_matchers::internal::Matcher<NamedDecl>> InMovedClassNames;
+  for (StringRef ClassName : ClassNames) {
+    llvm::StringRef GlobalClassName = ClassName.trim().ltrim(':');
+    const auto HasName = hasName(("::" + GlobalClassName).str());
+    InMovedClassNames =
+        InMovedClassNames ? anyOf(*InMovedClassNames, HasName) : HasName;
+  }
+  if (!InMovedClassNames) {
+    llvm::errs() << "No classes being moved.\n";
+    return;
+  }
+
   auto InOldHeader = isExpansionInFile(
       MakeAbsolutePath(OriginalRunningDirectory, Spec.OldHeader));
   auto InOldCC = isExpansionInFile(
       MakeAbsolutePath(OriginalRunningDirectory, Spec.OldCC));
   auto InOldFiles = anyOf(InOldHeader, InOldCC);
   auto InMovedClass =
-      hasDeclContext(cxxRecordDecl(hasName(FullyQualifiedName)));
+      hasDeclContext(cxxRecordDecl(*InMovedClassNames));
 
   // Match moved class declarations.
   auto MovedClass = cxxRecordDecl(
-      InOldFiles, hasName(FullyQualifiedName), isDefinition(),
+      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,
-                                   ofClass(hasName(FullyQualifiedName)),
+                                   ofClass(*InMovedClassNames),
                                    isDefinition())
                          .bind("class_method"),
                      this);
 
   // Match static member variable definition of the moved class.
   Finder->addMatcher(varDecl(InMovedClass, InOldCC, isDefinition())
                          .bind("class_static_var_decl"),
                      this);

clang-tools-extra/trunk/clang-move/tool/ClangMoveMain.cpp

if (std::error_code ec =
           llvm::sys::fs::openFileForWrite(path, fd, llvm::sys::fs::F_Text))
     return ec;
 
   return llvm::sys::Process::SafelyCloseFileDescriptor(fd);
 }
 
 cl::OptionCategory ClangMoveCategory("clang-move options");
 
-cl::opt<std::string> Name("name", cl::desc("The name of class being moved."),
-                          cl::cat(ClangMoveCategory));
+cl::opt<std::string>
+    Names("names", cl::desc("A comma-separated list of the names of classes "
+                            "being moved, e.g. \"Foo\", \"a::Foo, b::Foo\"."),
 
 cl::opt<std::string>
     OldHeader("old_header",
               cl::desc("The relative/absolute file path of old header."),
               cl::cat(ClangMoveCategory));
 
 cl::opt<std::string>
     OldCC("old_cc", cl::desc("The relative/absolute file path of old cc."),
for (size_t i = 0; i < ExtraArgs.size(); ++i)
     RawExtraArgs[i] = ExtraArgs[i].c_str();
   int Argc = argc + 1;
   tooling::CommonOptionsParser OptionsParser(Argc, RawExtraArgs.get(),
                                              ClangMoveCategory);
 
   tooling::RefactoringTool Tool(OptionsParser.getCompilations(),
                                 OptionsParser.getSourcePathList());
   move::ClangMoveTool::MoveDefinitionSpec Spec;
-  Spec.Name = Name;
+  Spec.Names = Names;
   Spec.OldHeader = OldHeader;
   Spec.NewHeader = NewHeader;
   Spec.OldCC = OldCC;
   Spec.NewCC = NewCC;
 
   llvm::SmallString<128> InitialDirectory;
   if (std::error_code EC = llvm::sys::fs::current_path(InitialDirectory))
     llvm::report_fatal_error("Cannot detect current path: " +

clang-tools-extra/trunk/test/clang-move/Inputs/database_template.json

 [
 {
   "directory": "$test_dir/build",
   "command": "clang++ -o test.o $test_dir/test.cpp",
   "file": "$test_dir/test.cpp"
+},
+{
+  "directory": "$test_dir/build",
+  "command": "clang++ -o test.o $test_dir/multiple_class_test.cpp",
+  "file": "$test_dir/multiple_class_test.cpp"
 }
 ]

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

+namespace a {
+class Move1 {
+public:
+  int f();
+};
+} // namespace a
+
+namespace b {
+class Move2 {
+public:
+  int f();
+};
+} // namespace b
+
+namespace c {
+class Move3 {
+public:
+  int f();
+};
+
+class Move4 {
+public:
+  int f();
+};
+
+class NoMove {
+public:
+  int f();
+};
+} // namespace c

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

+#include "multiple_class_test.h"
+
+namespace a {
+int Move1::f() {
+  return 0;
+}
+} // namespace a
+
+namespace b {
+int Move2::f() {
+  return 0;
+}
+} // namespace b
+
+namespace c {
+int Move3::f() {
+  return 0;
+}
+
+int Move4::f() {
+  return 0;
+}
+
+int NoMove::f() {
+  return 0;
+}
+} // namespace c

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

 // RUN: mkdir -p %T/clang-move/build
 // RUN: sed 's|$test_dir|%/T/clang-move|g' %S/Inputs/database_template.json > %T/clang-move/compile_commands.json
 // RUN: cp %S/Inputs/test*  %T/clang-move/
 // RUN: touch %T/clang-move/test2.h
 // RUN: cd %T/clang-move
-// RUN: clang-move -name="a::Foo" -new_cc=%T/clang-move/new_test.cpp -new_header=%T/clang-move/new_test.h -old_cc=../clang-move/test.cpp -old_header=../clang-move/test.h %T/clang-move/test.cpp
+// RUN: clang-move -names="a::Foo" -new_cc=%T/clang-move/new_test.cpp -new_header=%T/clang-move/new_test.h -old_cc=../clang-move/test.cpp -old_header=../clang-move/test.h %T/clang-move/test.cpp
 // RUN: FileCheck -input-file=%T/clang-move/new_test.cpp -check-prefix=CHECK-NEW-TEST-CPP %s
 // RUN: FileCheck -input-file=%T/clang-move/new_test.h -check-prefix=CHECK-NEW-TEST-H %s
 // RUN: FileCheck -input-file=%T/clang-move/test.cpp -check-prefix=CHECK-OLD-TEST-CPP %s
 // RUN: FileCheck -input-file=%T/clang-move/test.h %s -implicit-check-not='{{namespace.*}}'
 //
 // RUN: cp %S/Inputs/test*  %T/clang-move/
 // RUN: cd %T/clang-move
-// RUN: clang-move -name="a::Foo" -new_cc=%T/clang-move/new_test.cpp -new_header=%T/clang-move/new_test.h -old_cc=%T/clang-move/test.cpp -old_header=%T/clang-move/test.h %T/clang-move/test.cpp
+// RUN: clang-move -names="a::Foo" -new_cc=%T/clang-move/new_test.cpp -new_header=%T/clang-move/new_test.h -old_cc=%T/clang-move/test.cpp -old_header=%T/clang-move/test.h %T/clang-move/test.cpp
 // RUN: FileCheck -input-file=%T/clang-move/new_test.cpp -check-prefix=CHECK-NEW-TEST-CPP %s
 // RUN: FileCheck -input-file=%T/clang-move/new_test.h -check-prefix=CHECK-NEW-TEST-H %s
 // RUN: FileCheck -input-file=%T/clang-move/test.cpp -check-prefix=CHECK-OLD-TEST-CPP %s
 // RUN: FileCheck -input-file=%T/clang-move/test.h %s -implicit-check-not='{{namespace.*}}'
 //
 // CHECK-NEW-TEST-H: namespace a {
 // CHECK-NEW-TEST-H: class Foo {
 // CHECK-NEW-TEST-H: public:

clang-tools-extra/trunk/test/clang-move/move-multiple-classes.cpp

+// RUN: mkdir -p %T/clang-move/build
+// RUN: sed 's|$test_dir|%/T/clang-move|g' %S/Inputs/database_template.json > %T/clang-move/compile_commands.json
+// RUN: cp %S/Inputs/multiple_class_test*  %T/clang-move/
+// RUN: cd %T/clang-move
+// RUN: clang-move -names="a::Move1, b::Move2,c::Move3,c::Move4" -new_cc=%T/clang-move/new_multiple_class_test.cpp -new_header=%T/clang-move/new_multiple_class_test.h -old_cc=%T/clang-move/multiple_class_test.cpp -old_header=../clang-move/multiple_class_test.h %T/clang-move/multiple_class_test.cpp
+// RUN: FileCheck -input-file=%T/clang-move/new_multiple_class_test.cpp -check-prefix=CHECK-NEW-TEST-CPP %s
+// RUN: FileCheck -input-file=%T/clang-move/new_multiple_class_test.h -check-prefix=CHECK-NEW-TEST-H %s
+// RUN: FileCheck -input-file=%T/clang-move/multiple_class_test.cpp -check-prefix=CHECK-OLD-TEST-CPP %s
+// RUN: FileCheck -input-file=%T/clang-move/multiple_class_test.h -check-prefix=CHECK-OLD-TEST-H %s
+//
+// CHECK-OLD-TEST-H: namespace c {
+// CHECK-OLD-TEST-H: class NoMove {
+// CHECK-OLD-TEST-H: public:
+// CHECK-OLD-TEST-H:   int f();
+// CHECK-OLD-TEST-H: };
+// CHECK-OLD-TEST-H: } // namespace c
+
+// CHECK-OLD-TEST-CPP: #include "{{.*}}multiple_class_test.h"
+// CHECK-OLD-TEST-CPP: namespace c {
+// CHECK-OLD-TEST-CPP: int NoMove::f() {
+// CHECK-OLD-TEST-CPP:   return 0;
+// CHECK-OLD-TEST-CPP: }
+// CHECK-OLD-TEST-CPP: } // namespace c
+
+// CHECK-NEW-TEST-H: namespace a {
+// CHECK-NEW-TEST-H: class Move1 {
+// CHECK-NEW-TEST-H: public:
+// CHECK-NEW-TEST-H:   int f();
+// CHECK-NEW-TEST-H: };
+// CHECK-NEW-TEST-H: } // namespace a
+// CHECK-NEW-TEST-H: namespace b {
+// CHECK-NEW-TEST-H: class Move2 {
+// CHECK-NEW-TEST-H: public:
+// CHECK-NEW-TEST-H:   int f();
+// CHECK-NEW-TEST-H: };
+// CHECK-NEW-TEST-H: } // namespace b
+// CHECK-NEW-TEST-H: namespace c {
+// CHECK-NEW-TEST-H: class Move3 {
+// CHECK-NEW-TEST-H: public:
+// CHECK-NEW-TEST-H:   int f();
+// CHECK-NEW-TEST-H: };
+// CHECK-NEW-TEST-H: class Move4 {
+// CHECK-NEW-TEST-H: public:
+// CHECK-NEW-TEST-H:   int f();
+// CHECK-NEW-TEST-H: };
+// CHECK-NEW-TEST-H: } // namespace c
+
+// CHECK-NEW-TEST-CPP: #include "{{.*}}new_multiple_class_test.h"
+// CHECK-NEW-TEST-CPP: namespace a {
+// CHECK-NEW-TEST-CPP: int Move1::f() { return 0; }
+// CHECK-NEW-TEST-CPP: } // namespace a
+// CHECK-NEW-TEST-CPP: namespace b {
+// CHECK-NEW-TEST-CPP: int Move2::f() { return 0; }
+// CHECK-NEW-TEST-CPP: } // namespace b
+// CHECK-NEW-TEST-CPP: namespace c {
+// CHECK-NEW-TEST-CPP: int Move3::f() { return 0; }
+// CHECK-NEW-TEST-CPP: int Move4::f() { return 0; }
+// CHECK-NEW-TEST-CPP: } // namespace c

clang-tools-extra/trunk/unittests/clang-move/ClangMoveTests.cpp

for (const auto &It : FileToReplacements) {
     StringRef FilePath = It.first;
     Results[FilePath] = Context.getRewrittenText(FileToFileID[FilePath]);
   }
   return Results;
 }
 
 TEST(ClangMove, MoveHeaderAndCC) {
   move::ClangMoveTool::MoveDefinitionSpec Spec;
-  Spec.Name = "a::b::Foo";
+  Spec.Names = "a::b::Foo";
   Spec.OldHeader = "foo.h";
   Spec.OldCC = "foo.cc";
   Spec.NewHeader = "new_foo.h";
   Spec.NewCC = "new_foo.cc";
   std::string ExpectedHeader = "#include \"" + Spec.NewHeader + "\"\n\n";
   auto Results = runClangMoveOnCode(Spec);
   EXPECT_EQ(ExpectedTestHeader, Results[Spec.OldHeader]);
   EXPECT_EQ(ExpectedTestCC, Results[Spec.OldCC]);
   EXPECT_EQ(ExpectedNewHeader, Results[Spec.NewHeader]);
   EXPECT_EQ(ExpectedHeader + ExpectedNewCC, Results[Spec.NewCC]);
 }
 
 TEST(ClangMove, MoveHeaderOnly) {
   move::ClangMoveTool::MoveDefinitionSpec Spec;
-  Spec.Name = "a::b::Foo";
+  Spec.Names = "a::b::Foo";
   Spec.OldHeader = "foo.h";
   Spec.NewHeader = "new_foo.h";
   auto Results = runClangMoveOnCode(Spec);
   EXPECT_EQ(2u, Results.size());
   EXPECT_EQ(ExpectedTestHeader, Results[Spec.OldHeader]);
   EXPECT_EQ(ExpectedNewHeader, Results[Spec.NewHeader]);
 }
 
 TEST(ClangMove, MoveCCOnly) {
   move::ClangMoveTool::MoveDefinitionSpec Spec;
-  Spec.Name = "a::b::Foo";
+  Spec.Names = "a::b::Foo";
   Spec.OldCC = "foo.cc";
   Spec.NewCC = "new_foo.cc";
   std::string ExpectedHeader = "#include \"foo.h\"\n\n";
   auto Results = runClangMoveOnCode(Spec);
   EXPECT_EQ(2u, Results.size());
   EXPECT_EQ(ExpectedTestCC, Results[Spec.OldCC]);
   EXPECT_EQ(ExpectedHeader + ExpectedNewCC, Results[Spec.NewCC]);
 }
 
 TEST(ClangMove, MoveNonExistClass) {
   move::ClangMoveTool::MoveDefinitionSpec Spec;
-  Spec.Name = "NonExistFoo";
+  Spec.Names = "NonExistFoo";
   Spec.OldHeader = "foo.h";
   Spec.OldCC = "foo.cc";
   Spec.NewHeader = "new_foo.h";
   Spec.NewCC = "new_foo.cc";
   auto Results = runClangMoveOnCode(Spec);
   EXPECT_EQ(0u, Results.size());
 }
 
 } // namespace
 } // namespce move
 } // namespace clang