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