ioeric added a comment. Second rounds. Will start reviewing `CallGraph` code next.
================ Comment at: clang-move/ClangMove.cpp:492 + isDefinition(), unless(InMovedClass), InOldCC, + anyOf(isStaticStorageClass(), hasParent(namespaceDecl(isAnonymous())))); + auto HelperFuncOrVar = namedDecl(anyOf(functionDecl(IsOldCCHelperDefinition), ---------------- hokein wrote: > 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. I mean merging helper classes into `helperFuncOrVar` (maybe need to be renamed) so that class helpers are parallel with `functionDecl` and `varDecl` here. ================ Comment at: clang-move/ClangMove.cpp:459 //============================================================================ - auto InOldCCNamedOrGlobalNamespace = - allOf(hasParent(decl(anyOf(namespaceDecl(unless(isAnonymous())), - translationUnitDecl()))), - InOldCC); - // Matching using decls/type alias decls which are in named namespace or - // global namespace. Those in classes, functions and anonymous namespaces are - // covered in other matchers. + auto InOldCCNamespace = allOf( + hasParent(decl(anyOf(namespaceDecl(), translationUnitDecl()))), InOldCC); ---------------- I got the meaning here, but the name is a bit inaccurate since this also includes `TranslationUnitDecl`. ================ Comment at: clang-move/ClangMove.cpp:461 + hasParent(decl(anyOf(namespaceDecl(), translationUnitDecl()))), InOldCC); + // Matching using decls/type alias decls which are in named/anonymous/global + // namespace. Those in classes, functions are covered in other matchers. ---------------- Maybe also explain a bit on how you handle these using and alias decls here. Are they always copied/moved regardless where they are? ================ Comment at: clang-move/ClangMove.cpp:493 + varDecl(IsOldCCHelperDefinition))); + auto HelperClasses = + cxxRecordDecl(isDefinition(), unless(InMovedClass), InOldCC, ---------------- Thinking about this some more. Helpers might be (incorrectly) defined in non-anonymous namespaces without static qualifier. Do we want to consider these? ================ Comment at: clang-move/ClangMove.cpp:698 + // Filter out unused helper declarations. + // We only move the used helper declarations (including transversely used + // helpers) and the given symbols being moved. ---------------- transitively? ================ Comment at: clang-move/UsedHelperDeclFinder.cpp:20 +// outmost eclosing class declaration or function declaration if exists. +// Because we consider that all method declarations in a class are represented +// by a single node which belongs to that class. ---------------- s/Because.*$/We treat a class and all its members as one single node in the call graph/? ================ 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: ---------------- hokein wrote: > 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`. So, this should be named something like reference graph instead of call graph? Call graph might be confusing for readers without context. ================ Comment at: clang-move/UsedHelperDeclFinder.h:34 +// +// To construct the graph, we use AST matcher to find interested Decls (usually +// a pair of Caller and Callee), and add an edge from the Caller node to the ---------------- s/interested/interesting/ ================ Comment at: clang-move/UsedHelperDeclFinder.h:45 +// definition is in old.cc, the representing graph node for this function is +// associated with the FunctionDecl in old.h, because there are two +// different FunctionDecl pointers in the AST implementation, we want to make it ---------------- what is this `because...` associated with? https://reviews.llvm.org/D27673 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
