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

[Eric Liu via cfe-commits]

ioeric added inline comments.


> ClangMove.cpp:295
>  void ClangMoveTool::registerMatchers(ast_matchers::MatchFinder *Finder) {
> -  std::string FullyQualifiedName = "::" + Spec.Name;
> +  auto ParseNames = [](llvm::StringRef Names) {
> +    SmallVector<StringRef, 4> ClassNames;

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

> ClangMove.cpp:303
> +  for (StringRef ClassName : ClassNames) {
> +    const auto HasName = hasName(("::" + ClassName.ltrim(":")).str());
> +    if (InMovedClassNames)

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

> ClangMove.cpp:304
> +    const auto HasName = hasName(("::" + ClassName.ltrim(":")).str());
> +    if (InMovedClassNames)
> +      InMovedClassNames = anyOf(*InMovedClassNames, HasName);

Does this work?

  InMovedClassNames = InMovedClassNames ? ... : ...

> ClangMove.cpp:316
>    auto InMovedClass =
> -      hasDeclContext(cxxRecordDecl(hasName(FullyQualifiedName)));
> +      hasDeclContext(cxxRecordDecl(*InMovedClassNames));
>  

What would happen if `InMovedClassNames == false`?

> ClangMove.h:40
>    struct MoveDefinitionSpec {
> -    // A fully qualified name, e.g. "X", "a::X".
> -    std::string Name;
> +    // A semicolon-separated list of fully qualified names, e.g. "Foo",
> +    // "a::Foo;b::Foo".

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

> ClangMoveMain.cpp:41
> +cl::opt<std::string>
> +    Name("name", cl::desc("A semicolon-separated list of the names of classes "
> +                          "being moved, e.g. \"Foo\", \"a::Foo;b::Foo\"."),

Same here if semi->comma.

And I'd name this `Names` and `names`

> multiple_class_test.cpp:15
> +
> +namespace c {
> +int Bar::f() {

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

https://reviews.llvm.org/D25309