hokein added inline comments.
================
Comment at: clang-move/ClangMove.cpp:492
+ isDefinition(), unless(InMovedClass), InOldCC,
+ anyOf(isStaticStorageClass(), hasParent(namespaceDecl(isAnonymous()))));
+ auto HelperFuncOrVar = namedDecl(anyOf(functionDecl(IsOldCCHelperDefinition),
----------------
ioeric wrote:
> It seems that `isStaticStorageClass` is preventing combining matchers for
> functions, variables, and classes. Maybe only apply this matcher on
> `functionDecl` and `varDecl` below, so that helper classes can be matched
> with the same matcher?
Seems that it is hard to reuse the same matcher for `functionDecl`/`varDecl`
and `CXXRecordDecl` since `isStaticStorageClass` is not available to
`CXXRecordDecl`. So we have to match helper classes, helper functions/vars
separately. Have cleaned the code to make it clearer.
================
Comment at: clang-move/ClangMove.cpp:587
+ } else if (const auto *UD = Result.Nodes.getNodeAs<clang::NamedDecl>(
+ "using_decl_in_anonymous_ns")) {
+ MovedDecls.push_back(UD);
----------------
ioeric wrote:
> What about using declarations in non-anonymous namespaces in old cc? Do we
> also move those?
Those using-decls in named namespace are covered in "using_decl" (see above
`if` statement). Have combined them together.
================
Comment at: clang-move/ClangMove.cpp:627
+ // If old_header is not specified (only move declarations from old.cc),
remain
+ // all the helper function declarations in old.cc as UnremovedDecls is empty
+ // in this case.
----------------
ioeric wrote:
> Why is `UnremovedDecls` empty in this case btw?
In this case , `UnremovedDeclsInOldHeader` is empty, there is no way to verify
unused/used helper declarations.
================
Comment at: clang-move/ClangMove.cpp:715
+ if (!llvm::is_contained(HelperDeclarations, D) ||
+ UsedHelperDeclFinder::isUsed(D, HelperDecls))
+ RealNewCCDecls.push_back(D);
----------------
ioeric wrote:
> IIUC, this condition makes sure helpers used by helpers are moved. If so,
> please explain this in the comment.
Yes.
================
Comment at: clang-move/ClangMove.h:166
std::vector<std::string> CCIncludes;
+ // Records all helper declarations (functions/variables declared as static or
+ // declared in anonymous namespace) in old.cc, saving in an AST-visited
order.
----------------
ioeric wrote:
> Is helper class considered here?
Yes. Have made the comment a bit clearer.
================
Comment at: clang-move/UsedHelperDeclFinder.h:22
+
+// Call graph for helper declarations in a single translation unit e.g. old.cc.
+// Helper declarations include following types:
----------------
ioeric wrote:
> What's the relationship between this and the `CallGraph` class in
> `clang/Analysis/CallGraph.h`?
There is no relationship between them. We build our own CallGraph class to meet
our use cases. The CallGraph in `clang/Analysis` only supports function decls,
and it seems hard to reuse it. The thing we reuse is the `CallGraphNode`.
================
Comment at: clang-move/UsedHelperDeclFinder.h:25
+// * function/variable/class definitions in an anonymous namespace.
+// * static function/variable definitions in a global namespace.
+//
----------------
ioeric wrote:
> What about static decls in named namespaces? I think they can also be helpers
> right?
Yeah, it is already covered. Corrected the confusing comment.
================
Comment at: clang-move/UsedHelperDeclFinder.h:49
+ // D's node, including D.
+ llvm::DenseSet<const CallGraphNode *> getConnectedNodes(const Decl *D) const;
+
----------------
ioeric wrote:
> What does `connected` mean in this context? The graph is directed; does this
> mean reachable from D or to D?
renamed it to `getReachableNodes`.
https://reviews.llvm.org/D27673
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
