[PATCH] D26236: [clang-move] Move all codes from old.h/cc if old.h only contains one moved declaration.

[Eric Liu via cfe-commits]

ioeric added a comment.

First round of comments.

The patch summary says "one moved declaration."  Why it is just one moved declaration? Doesn't this also support moving all classes in one file?

Maybe also mention the new behavior in the class comment.



================
Comment at: clang-move/ClangMove.h:72
   /// \param FileName The name of file where the IncludeHeader comes from.
+  /// \param IncludeRange The source range for the old.h of #include in old.cc.
   /// \param SM The SourceManager.
----------------
It took me a while to understand this comment...

IIUC, this is the source range of the #include directive of old.h (i.e. #include "old.h") in old.cc?


================
Comment at: clang-move/ClangMove.h:109
+  // The source range for the old.h of #include in old.cc, including the
+  // enclosing quotes or angle brackets.
+  clang::CharSourceRange OldHeaderIncludeRange;
----------------
What about trailing comments or whitespaces?


================
Comment at: test/clang-move/Inputs/test.h:3
+#define TEST_H
 namespace a {
 class Foo {
----------------
Maybe add some white spaces or something in the file to demonstrate that the file is copied over.


================
Comment at: unittests/clang-move/ClangMoveTests.cpp:267
 
+TEST(ClangMove, MoveAll) {
+  move::ClangMoveTool::MoveDefinitionSpec Spec;
----------------
Can you add tests for move all (multiple) classes in file?

Not related to this patch...but it seems that we are missing test case for moving template class? 


================
Comment at: unittests/clang-move/ClangMoveTests.cpp:278
+  const char Code[] = "#include \"foo.h\"\nint A::f() { return 0; }";
+  Spec.Names = {std::string("A")};
+  Spec.OldHeader = "foo.h";
----------------
Maybe just insert or push back?


================
Comment at: unittests/clang-move/ClangMoveTests.cpp:280
+  Spec.OldHeader = "foo.h";
+  Spec.OldCC = "foo.cc";
+  Spec.NewHeader = "new_foo.h";
----------------
Where is foo.cc?


================
Comment at: unittests/clang-move/ClangMoveTests.cpp:299
+    "using Int=int;\nclass A {\npublic:\n  int f();\n};",
+    "namespace a {}\nusing namespace a;\nclass A {\npublic:\n  int f();\n};",
+    "class B {};\nclass A {\npublic:\n  int f();\n};",
----------------
I think using namespace decl should also be ignored?


https://reviews.llvm.org/D26236