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

[Artem Dergachev via Phabricator via cfe-commits]

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