[PATCH] D27673: [clang-move] Only move used helper declarations.

[Eric Liu via Phabricator via cfe-commits]

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