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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
