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