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

[Haojian Wu via Phabricator via cfe-commits]

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:
> 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.
We need to match helper classes separately because we want to reuse these two matchers (HelperFuncOrVar, HelperClasses) in finding helpers' usage below.


================
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);
----------------
ioeric wrote:
> I got the meaning here, but the name is a bit inaccurate since this also includes `TranslationUnitDecl`.
but`TranslationUnitDecl` also means in global namespace, right?


================
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.
----------------
ioeric wrote:
> Maybe also explain a bit on how you handle these using and alias decls here.
> 
> Are they always copied/moved regardless where they are?
Yeah, added comment.


================
Comment at: clang-move/ClangMove.cpp:493
+                                         varDecl(IsOldCCHelperDefinition)));
+  auto HelperClasses =
+      cxxRecordDecl(isDefinition(), unless(InMovedClass), InOldCC,
----------------
ioeric wrote:
> Thinking about this some more. Helpers might be (incorrectly) defined in non-anonymous namespaces without static qualifier. Do we want to consider these?
hmm, in theory, "helpers" defined accidently in non-anonymous namespaces are not helpers as they are visible outside of the current TU. The implementation relies on the invisible property of helpers to verify whether a function/variable is a helper, so this would be hard to achieve.


================
Comment at: clang-move/UsedHelperDeclFinder.cpp:22
+// by a single node which belongs to the class.
+const Decl *getOutmostEnclosingClassOrFunDecl(const Decl *D) {
+  const auto *DC = D->getDeclContext();
----------------
ioeric wrote:
> If we always only match outermost decls, we might be able to get rid of this. I feel that this kind of function (that traverse up AST) can easily hit into corner cases.
Beside construction of the graph, this function is also used in checking whether a given Declaration is used or not.


================
Comment at: clang-move/UsedHelperDeclFinder.cpp:22
+// by a single node which belongs to that class.
+const Decl *getOutmostEnclosingClassOrFunDecl(const Decl *D) {
+  const auto *DC = D->getDeclContext();
----------------
ioeric wrote:
> Maybe just `getOutermostDecls` and add FIXME for other data types.
The name might be too long, I have renamed it to `getOutermostClassOrFunDecls` (I prefer to keep the `ClassOrFunDecl` postfix which clearly indicates what this function exactly does without reading the comment, I'm happy to rename it in the future if needed).

Is there any types we want to support? From my understanding, it is sufficient to cover our usage now.


================
Comment at: clang-move/UsedHelperDeclFinder.cpp:30
+      Result = FD;
+      if (const auto *RD = dyn_cast<CXXRecordDecl>(FD->getParent()))
+        Result = RD;
----------------
ioeric wrote:
> Why do you need this? And when is a function's parent a class?
This is for the template method in a class.


================
Comment at: clang-move/UsedHelperDeclFinder.cpp:103
+
+UsedHelperDeclFinder::HelperDeclsSet UsedHelperDeclFinder::getUsedHelperDecls(
+    const std::vector<const NamedDecl *> &Decls) const {
----------------
ioeric wrote:
> Do you assume that all nodes/decls in the graph are helpers?
> 
> What if some moved `Decl` refers to unmoved decls?
The node in the graph can present moved/unmoved decls and helpers.

> What if some moved Decl refers to unmoved decls?

The graph doesn't contain this information, it only contains the reference  between moved/unmoved decls and helpers. So in this case, we just move the moved Decl.


================
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:
> 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.
But the "HelperDecl" indicates our special use case. Might be "HelperDeclGraph" or "HelperDeclDependencyGraph"?


https://reviews.llvm.org/D27673