[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:
> 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