lebedev.ri added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:21 + +inline bool operator==(const CallGraphNode::CallRecord &LHS, + const CallGraphNode::CallRecord &RHS) { ---------------- NoQ wrote: > 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? Now that i store `Expr*` in `CallGraphNode::CallRecord`, this comparison operator appears to be automatically provided, which means we really should provide our own to avoid confusing behavior as compared to the DenseMap behavior. So i'd say yes. ================ Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:31 + +// Specialize DenseMapInfo for clang::CallGraphNode::CallRecord. +template <> struct DenseMapInfo<clang::CallGraphNode::CallRecord> { ---------------- NoQ wrote: > 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) A good question, for another time. ================ Comment at: clang/lib/Analysis/CallGraph.cpp:103-106 // Include the evaluation of the default argument. void VisitCXXDefaultArgExpr(CXXDefaultArgExpr *E) { Visit(E->getExpr()); } ---------------- NoQ wrote: > 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. Test case added (is this how you'd imagine it to be a problem?). I'm not sure what you mean by a problem, it is diagnosed at least. 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