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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

First round of comments.



================
Comment at: clang-move/ClangMove.cpp:492
+      isDefinition(), unless(InMovedClass), InOldCC,
+      anyOf(isStaticStorageClass(), hasParent(namespaceDecl(isAnonymous()))));
+  auto HelperFuncOrVar = namedDecl(anyOf(functionDecl(IsOldCCHelperDefinition),
----------------
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?


================
Comment at: clang-move/ClangMove.cpp:495
+                                         varDecl(IsOldCCHelperDefinition)));
+  auto HelperClasses = cxxRecordDecl(
+      InOldCC, isDefinition(), hasDeclContext(namespaceDecl(isAnonymous())));
----------------
Why do we need to match classes separately? (Please explain in comments.)


================
Comment at: clang-move/ClangMove.cpp:514
+                                  hasParent(namespaceDecl(isAnonymous())));
+  auto DeclMatcher =
+      hasDeclaration(cxxRecordDecl(IsOldCCHelperClass).bind("used_class"));
----------------
ClassDeclMatcher?


================
Comment at: clang-move/ClangMove.cpp:519
+                             unless(hasAncestor(namespaceDecl(isAnonymous()))),
+                             hasAncestor(decl().bind("dc"))),
+                     &CGBuilder);
----------------
Can we just restrict `"dc"` to be the top level or outermost decls in the first place? So that we can guarantee all declarations we are dealing with are those that we really care about?  This would simplify the problem a bit I believe; otherwise, my gut feeling tells me there would be a lot of things that might go wrong.


================
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);
----------------
What about using declarations in non-anonymous namespaces in old cc? Do we also move those?


================
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.
----------------
Why is `UnremovedDecls` empty in this case btw?


================
Comment at: clang-move/ClangMove.cpp:637
+    // declarations.
+    UsedHelperDeclFinder::HelperDeclsSet UsedHelperDecls =
+        HelperDeclFinder.getUsedHelperDecls(UnremovedDecls);
----------------
Maybe a better name for this variable. It's a bit hard to follow which decls use these helper decls since there are several sets of decls here like moved/removed/unremoved...


================
Comment at: clang-move/ClangMove.cpp:708
+  UsedHelperDeclFinder HelperDeclFinder(CGBuilder.getCallGraph());
+  llvm::DenseSet<const Decl *> HelperDecls =
+      HelperDeclFinder.getUsedHelperDecls(RemovedDecls);
----------------
Maybe `RemovedDeclHelpers`?


================
Comment at: clang-move/ClangMove.cpp:715
+    if (!llvm::is_contained(HelperDeclarations, D) ||
+        UsedHelperDeclFinder::isUsed(D, HelperDecls))
+      RealNewCCDecls.push_back(D);
----------------
IIUC, this condition makes sure helpers used by helpers are moved. If so, please explain this in the comment.


================
Comment at: clang-move/ClangMove.h:93
+// files (old.h/cc) to new files (new.h/cc).
+// The goal of this tool is to make the new/old files as compliable as possible.
+// When moving a class, all its/ members are also moved. In addition,
----------------
s/compliable/compilable/


================
Comment at: clang-move/ClangMove.h:95
+// When moving a class, all its/ members are also moved. In addition,
+// all used helper declarations (functions/variables/class definitions in
+// anonymous namespace, static funtioncs/variables), using-declarations in
----------------
This also holds for functions. Maybe "when moving a symbol"

"all helper declarations ... used by moved symbols."


================
Comment at: clang-move/ClangMove.h:100
+//
+// The remaing unused helper declarations in old.cc after moving out the given
+// declarations will also be removed.
----------------
Maybe "remaining  helpers that are not used by non-moved symbols"?


================
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.
----------------
Is helper class considered here? 


================
Comment at: clang-move/UsedHelperDeclFinder.cpp:20
+// outmost eclosing class declaration or function declaration if exists.
+// Because we consider all class method declarations of a class are represented
+// by a single node which belongs to the class.
----------------
This sentence doesn't parse?


================
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();
----------------
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.


================
Comment at: clang-move/UsedHelperDeclFinder.cpp:107
+        CG->getConnectedNodes(getOutmostEnclosingClassOrFunDecl(RD));
+    for (const auto *N : Result)
+      Nodes.insert(N);
----------------
Doesn't `llvm::DenseSet` have range insertion?


================
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:
----------------
What's the relationship between this and the `CallGraph` class in `clang/Analysis/CallGraph.h`?


================
Comment at: clang-move/UsedHelperDeclFinder.h:25
+//   * function/variable/class definitions in an anonymous namespace.
+//   * static function/variable definitions in a global namespace.
+//
----------------
What about static decls in named namespaces? I think they can also be helpers right?


================
Comment at: clang-move/UsedHelperDeclFinder.h:28
+// Each node in the graph is representing a helper declaration in old.cc or
+// a non-moved/moved declaration in old.h.
+// The graph has 3 types of edges:
----------------
Are non-moved/moved declarations in old.cc considered as nodes?


================
Comment at: clang-move/UsedHelperDeclFinder.h:49
+  // D's node, including D.
+  llvm::DenseSet<const CallGraphNode *> getConnectedNodes(const Decl *D) const;
+
----------------
What does `connected` mean in this context? The graph is directed; does this mean reachable from D or to D?


https://reviews.llvm.org/D27673