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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
