Details
Diff Detail
- Build Status
Buildable 199 Build 199: arc lint + arc unit
Event Timeline
clang-move/ClangMove.cpp | ||
---|---|---|
295 | Why create this lambda if it's only used once? I think you can save a few lines by just inlining this. | |
303 | I'd also allow whitespaces around separators, e.g. a::b::X, a::b::Y. Maybe also trim() the name. | |
304 | Does this work? InMovedClassNames = InMovedClassNames ? ... : ... | |
316 | What would happen if InMovedClassNames == false? | |
clang-move/ClangMove.h | ||
40 | I'd use commas as separator. Eyeballs can easily mix ";" with ":". | |
clang-move/tool/ClangMoveMain.cpp | ||
41 | Same here if semi->comma. And I'd name this Names and names | |
test/clang-move/Inputs/multiple_class_test.cpp | ||
15 | Can you add one more moved class that is also in namespace c? |
Address review comments.
clang-move/ClangMove.cpp | ||
---|---|---|
316 | "InMovedClassNames" should not be false in the matcher. Added an assert. |
clang-move/ClangMove.cpp | ||
---|---|---|
299 | The comment is trivial. | |
302 | Can we use trim() with default argument? | |
316 | I think you should instead exit early if ClassNames.empty() or assert not empty. | |
test/clang-move/Inputs/multiple_class_test.cpp | ||
20 | What I wanted to see is another *moved* class because there was no test case where multiple classes are moved into the same namespace. |
I'd use commas as separator. Eyeballs can easily mix ";" with ":".