NoQ added a comment.
`CallGraph` changes look nice, i don't see any immediate problems and it's a
nice-to-have thing that other users may benefit from (the static analyzer
wouldn't though, it's only interested in topological sorting order on the
graph). I guess you could have as well stored a `CallExpr` with the
`CallRecord`.
Note that there are certain problems with the call graph itself that are caused
by how our very AST is structured. Hopefully they won't cause you too much
trouble. I outlined some of them in the inline comments.
================
Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:21
+
+inline bool operator==(const CallGraphNode::CallRecord &LHS,
+ const CallGraphNode::CallRecord &RHS) {
----------------
lebedev.ri wrote:
> JonasToth wrote:
> > That should be in the `CallGraph` code, adding a private operator overload
> > does not feel right.
> I didn't want to put this into callgraph, because this
> cements the fact that we do not care about sourceloc,
> which may not be true for other users.
> I can put it there, but if told so by it's code owners (@NoQ ?)
Does this need to be `operator==`? It looks like `DenseMap` uses
`DenseMapInfo::isEqual` instead, maybe just call it "isEqual" to avoid
confusion?
================
Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:31
+
+// Specialize DenseMapInfo for clang::CallGraphNode::CallRecord.
+template <> struct DenseMapInfo<clang::CallGraphNode::CallRecord> {
----------------
lebedev.ri wrote:
> JonasToth wrote:
> > That stuff, too.
> (same reasoning)
So what exactly happens if you link together two different translation units
with different specializations of `DenseMapInfo<CallRecord>` and start passing
such dense maps from one TU to the other?
(ODR violations are definitely not my specialty)
================
Comment at: clang/lib/Analysis/CallGraph.cpp:96-101
void VisitCXXConstructExpr(CXXConstructExpr *E) {
CXXConstructorDecl *Ctor = E->getConstructor();
if (FunctionDecl *Def = Ctor->getDefinition())
- addCalledDecl(Def);
+ addCalledDecl(Def, Ctor->getBeginLoc());
VisitChildren(E);
}
----------------
Note that you're missing out on destructors. They aren't in the call graph
because they aren't in the AST to begin with. Like, declarations are there of
course, but expressions are mostly missing. A destructor may call another
function that will in turn destroy an object of the same type (maybe even the
same object), causing recursion.
(side note: we're missing out on destructors too)
================
Comment at: clang/lib/Analysis/CallGraph.cpp:103-106
// Include the evaluation of the default argument.
void VisitCXXDefaultArgExpr(CXXDefaultArgExpr *E) {
Visit(E->getExpr());
}
----------------
Note that if a function `void foo(int x = boo()) { ... }` is invoked as `void
bar() { foo(); foo(); }`, you'd get two different-in-every-way call records
from the same function `bar()` to the same function `boo()` with the same
source location (the location of call-expression `boo()` within the
`ParmVarDecl` for `x`). I'm legit curious if this is a problem for your checker.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72362/new/
https://reviews.llvm.org/D72362
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
