[PATCH] D72362: [clang-tidy] misc-no-recursion: a new check

[Roman Lebedev via Phabricator via cfe-commits]

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