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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

So that is were the CTU question comes from? :)



================
Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:21
+
+inline bool operator==(const CallGraphNode::CallRecord &LHS,
+                       const CallGraphNode::CallRecord &RHS) {
----------------
That should be in the `CallGraph` code, adding a private operator overload does not feel right.


================
Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:31
+
+// Specialize DenseMapInfo for clang::CallGraphNode::CallRecord.
+template <> struct DenseMapInfo<clang::CallGraphNode::CallRecord> {
----------------
That stuff, too.


================
Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:67
+// 2. it is immutable, the way it was constructed it will stay.
+template <typename T, unsigned SmallSize> class ImmutableSmartSet {
+  ArrayRef<T> Vector;
----------------
Why `smart`?


================
Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:73
+
+  bool isSmall() const { return Set.empty(); }
+
----------------
That method name looks confusing. `isEmpty()`? If not, why?


================
Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:242
+    Decl *D = N->getDecl();
+    diag(D->getLocation(), "function '%0' is part of call graph loop")
+        << cast<NamedDecl>(D)->getName();
----------------
That should be a `Note:`


================
Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:287
+  diag(CyclicCallStack.back().Loc,
+       "... which was the starting point of this call graph",
+       DiagnosticIDs::Note);
----------------
Please merge these two notes into one.


================
Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.h:28
+class NoRecursionCheck : public ClangTidyCheck {
+  void handleSCC(ArrayRef<CallGraphNode *> SCC);
+
----------------
nit: the `private` stuff is usually at the bottom in the other clang-tidy code. not sure if there is something in the coding standard though.


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:149
+
+  Finds strongly connected functions (by analyzing call graph for SCC's
+  that are loops), diagnoses each function in the cycle,
----------------
The technical details should not be in the release notes. Just that it finds recursion and diagnoses it.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/misc-no-recursion.cpp:127
+}
+
+// CHECK-NOTES: :[[@LINE-14]]:13: warning: function 'indirect_recursion_with_depth2' is part of call graph loop [misc-no-recursion]
----------------
Does the check find recursion through function pointers? (Probably not? Should be noted as limitation).

Please add cases from lambdas. And cases where recursion happens in templates / only with one of multiple instantiations.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72362/new/

https://reviews.llvm.org/D72362