[PATCH] D25309: [clang-move] Support moving multiple classes in one run.
Eric Liu via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 6 10:47:59 PDT 2016
ioeric added inline comments.
> hokein wrote in ClangMove.cpp:316
> "InMovedClassNames" should not be false in the matcher. Added an assert.
I think you should instead exit early if `ClassNames.empty()` or assert not empty.
> ClangMove.cpp:299
> + for (StringRef ClassName : ClassNames) {
> + // Removed the preceding and trailing space of ClassName.
> + // And removed the global scope operator "::" since we always regard the
The comment is trivial.
> ClangMove.cpp:302
> + // ClassName as a global name.
> + llvm::StringRef GlobalClassName = ClassName.trim(' ').ltrim(':');
> + const auto HasName = hasName(("::" + GlobalClassName).str());
Can we use `trim()` with default argument?
> multiple_class_test.cpp:19
> +}
> +int NoMove::f() {
> + return 0;
What I wanted to see is another *moved* class because there was no test case where multiple classes are moved into the same namespace.
https://reviews.llvm.org/D25309
More information about the cfe-commits
mailing list