[PATCH] D26236: [clang-move] Move all code from old.h/cc directly when moving all class declarations from old.h.

[Haojian Wu via cfe-commits]

hokein added a comment.

> 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?

Oh, the patch summary is not totally correct. It actually does what you mention above. Have updated the summary.



================
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.
----------------
ioeric wrote:
> 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?
Sorry for the confusion.

Indeed this a range for the written file name of `#include`, i.e. if the #include is `#include "old.h"`, then the range here is for `"old.h"`. Have updated the comment.


================
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;
----------------
ioeric wrote:
> What about trailing comments or whitespaces?
The trailing comments or whitespaces will be ignored here. We just copy this field from `InclusionDirective` interface, and seems there is no explicit way to retrieve the comments or whitespaces.


================
Comment at: unittests/clang-move/ClangMoveTests.cpp:267
 
+TEST(ClangMove, MoveAll) {
+  move::ClangMoveTool::MoveDefinitionSpec Spec;
----------------
ioeric wrote:
> 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? 
> Not related to this patch...but it seems that we are missing test case for moving template class?

Yeah, moving template classes doesn't support perfectly right now.


================
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";
----------------
ioeric wrote:
> Maybe just insert or push back?
I think using `insert` or `push_back` doesn't make the code as clear as initialization here. Code readers might pull up the source file to find the code for the initialized value of this variable.


================
Comment at: unittests/clang-move/ClangMoveTests.cpp:280
+  Spec.OldHeader = "foo.h";
+  Spec.OldCC = "foo.cc";
+  Spec.NewHeader = "new_foo.h";
----------------
ioeric wrote:
> Where is foo.cc?
The `Code` variable above represents the source code of `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};",
----------------
ioeric wrote:
> I think using namespace decl should also be ignored?
This case should rarely happen in source code, as many code styles prohibit `using-namespace` decls in headers.
I'd like to keep the current behavior because the using-namespace decl will populate the namespace in all the files which include this header. Ignoring it might break a lot of code.


https://reviews.llvm.org/D26236