[PATCH] D24243: [clang-move] A prototype tool for moving class definition to new file.

[Eric Liu via cfe-commits]

ioeric added inline comments.

================
Comment at: clang-move/ClangMove.cpp:103
@@ +102,3 @@
+                                     const clang::SourceManager *SM) {
+  // Gets the ending ";".
+  auto EndLoc = clang::Lexer::getLocForEndOfToken(D->getLocEnd(), 0, *SM,
----------------
hokein wrote:
> ioeric wrote:
> > Consider the code below to also include trailing comment.
> >     clang::SourceLocation after_semi = clang::Lexer::findLocationAfterToken(
> >         D->getLocEnd, clang::tok::semi, *SM,
> >         result.Context->getLangOpts(),
> >         /*SkipTrailingWhitespaceAndNewLine=*/true);
> > 
> But this code could not handle cases where the declaration definition has no semi at the end, such as "void f() {}"
Please see the comment for `findLocationAfterToken`.



> If the token is not found or the location is inside a macro, the returned source location will be invalid.




================
Comment at: clang-move/ClangMove.cpp:129
@@ +128,3 @@
+  for (const auto &MovedDecl : Decls) {
+    std::vector<std::string> DeclNamespaces = GetNamespaces(MovedDecl.Decl);
+    auto CurrentIt = CurrentNamespaces.begin();
----------------
hokein wrote:
> ioeric wrote:
> > Is it possible to restrict all `MovedDecl.Decl` to NamedDecl so that we can use `getQualifiedNameAsString` instead of having a second implementation of retrieving namespaces. 
> > 
> > Also how is anonymous namespace handled here?
> > 
> > 
> Yeah, `getQualifiedNameAsString` looks like a simpler way, but it doesn't suitable for our usage here.
> 
> The `getQualifiedNameAsString` includes the name of the class.  For instance, a class method decl like `void A::f() {}` defined in global namespace, it retruns `A::f` which is not intended. 
> 
> It handles anonymous namespace by wrapping like `namespace { ... } // namespace`, see the test.
> 
I think it should be relatively easy to split the qualified name returned by `getQualifiedNameAsString `.

> It handles anonymous namespace by wrapping like namespace { ... } // namespace, see the test.
I mean...wouldn't `getNamespaces` return something like "a::(anonymous)" for anonymous namespace? How do you handle this?


================
Comment at: clang-move/ClangMove.h:39
@@ +38,3 @@
+  struct MoveDefinitionSpec {
+    std::string Name;
+    std::string OldHeader;
----------------
hokein wrote:
> ioeric wrote:
> > Should the `Name` be fully qualified?
> It's not required. The name can be fully/partially qualified, e.g., `::a::b::X`, `a::b::X`, `b::X`, `X`, which has the same behavior with `hasName` ast matcher.
> 
> It the given name is partially qualified, it will match all the class whose name ends with the given name.
> It the given name is partially qualified, it will match all the class whose name ends with the given name.
In this case, all matched classes will be moved right? Might want to mention this in the comment.

================
Comment at: clang-move/ClangMove.h:65
@@ +64,3 @@
+
+  const MoveDefinitionSpec &Spec;
+  // The Key is file path, value is the replacements being applied to the file.
----------------
hokein wrote:
> ioeric wrote:
> > Is there any reason why you made this a reference?
> To avoid the cost of making a copy of the structure.
If the copy is not that expensive, I'd just make a copy so that I don't need to worry about the life-cycle of `Spec`.


https://reviews.llvm.org/D24243