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

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
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();
----------------
Maybe just `getOutermostDecls` and add FIXME for other data types.


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


================
Comment at: clang-move/UsedHelperDeclFinder.cpp:103
+
+UsedHelperDeclFinder::HelperDeclsSet UsedHelperDeclFinder::getUsedHelperDecls(
+    const std::vector<const NamedDecl *> &Decls) const {
----------------
Do you assume that all nodes/decls in the graph are helpers?

What if some moved `Decl` refers to unmoved decls?


================
Comment at: clang-move/UsedHelperDeclFinder.cpp:125
+    const ast_matchers::MatchFinder::MatchResult &Result) {
+  if (const clang::DeclRefExpr *FR =
+          Result.Nodes.getNodeAs<clang::DeclRefExpr>("func_ref")) {
----------------
I find abbreviations hurt readability in general. Maybe just `FuncRef`.


================
Comment at: clang-move/UsedHelperDeclFinder.cpp:128
+    const auto *DC = Result.Nodes.getNodeAs<clang::Decl>("dc");
+    if (!DC)
+      return;
----------------
When would `DC` be null? Shouldn't we assert this instead of ignoring?


================
Comment at: clang-move/UsedHelperDeclFinder.cpp:132
+    // definition.
+    const auto *T = DC->getPreviousDecl();
+    if (!T)
----------------
Shouldn't we use `getCanonicalDecl` instead so that we can always get the same Decl for all canonical declaration?

And if you are doing this when adding nodes, shouldn't you also do `getPreviousDecl` or `getCanonicalDecl` for `getNode` and `getOrInsertNode` etc?


================
Comment at: clang-move/UsedHelperDeclFinder.cpp:145
+    // whose outmost enclosing declaration is the "CLASS" CXXRecord Declaration.
+    if (T == D)
+      return;
----------------
I'd move this check into `addNodeForDecl`.


================
Comment at: clang-move/UsedHelperDeclFinder.h:59
+  // Add a directed edge from the caller node to the callee node.
+  // A new node will be created if the node for Caller/Callee isn't existed.
+  //
----------------
s/isn't existed/doesn't exist/


================
Comment at: clang-move/UsedHelperDeclFinder.h:61
+  //
+  // Note that, all methods/variables declarations of a class are represented by
+  // a single node in the graph. The corresponding Decl of this node is the
----------------
maybe just "all members"?


================
Comment at: clang-move/UsedHelperDeclFinder.h:64
+  // class declaration.
+  void addNodeForDecl(const Decl *Caller, const Decl *Callee);
+  CallGraphNode *getNode(const Decl *D) const;
----------------
So, this is not adding node?


================
Comment at: clang-move/UsedHelperDeclFinder.h:107
+  HelperDeclsSet
+  getUsedHelperDecls(const std::vector<const NamedDecl *> &Decls) const;
+
----------------
Maybe add `FIXME` somewhere in the code for other kinds of declarations.


================
Comment at: clang-move/UsedHelperDeclFinder.h:110
+  // Check whether the given declaration D is in the HelperDeclsSet.
+  static bool isUsed(const Decl *D, const HelperDeclsSet &UsedDecls);
+
----------------
This function is quite confusing for me. The comment indicates that this checks if `D` is in `UsedDecls`?  But the implementation does more than that. I think we need a better name and comment for this.


https://reviews.llvm.org/D27673