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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Code is almost good. I'm just still a bit confused by names.



================
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);
----------------
hokein wrote:
> 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?
Maybe, but it is not straight-forward. I think what you really want here is top-level declarations in old cc. The name does not really imply that. For example, something defined in a function in a namespace is also in that namespace.


================
Comment at: clang-move/UsedHelperDeclFinder.cpp:30
+      Result = FD;
+      if (const auto *RD = dyn_cast<CXXRecordDecl>(FD->getParent()))
+        Result = RD;
----------------
hokein wrote:
> 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.
Wouldn't that be handled in the next iteration?

Also, if you do another `getParent` here, do you also need to update `DC`?


================
Comment at: clang-move/UsedHelperDeclFinder.cpp:103
+
+UsedHelperDeclFinder::HelperDeclsSet UsedHelperDeclFinder::getUsedHelperDecls(
+    const std::vector<const NamedDecl *> &Decls) const {
----------------
hokein wrote:
> 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.
So, IIUC, the graph can contain both non-helper decls and helper decls. In that case, why do we name it `HelperDecl` Graph? And this `getUsedHelperDecls` does not make sense either. Would be less confusing if it is just `getUsedDecls`.


================
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:
> > 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"?
The problem is not with `HelperDecl` part. It's the `CallGraph` part that's a bit confusing. I think `HelperDeclReferenceGraph` might be better?


================
Comment at: clang-move/UsedHelperDeclFinder.h:110
+  // before checking it in UsedDecls.
+  static bool isUsed(const Decl *D, const HelperDeclsSet &UsedDecls);
+
----------------
This is still a bit confusing even with the comment.

I think the confusing part is `Used`, which makes it hard to understand what this function does, and I think the reason why it is hard to find a good name might due to the wrong abstraction here.  Maybe just inline this function? It is just one line after all.


https://reviews.llvm.org/D27673