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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:28
+// 2. it is immutable, the way it was constructed it will stay.
+template <typename T, unsigned SmallSize> class ImmutableSmartSet {
+  ArrayRef<T> Vector;
----------------
"Smart" is not a descriptive term. This seems like it's an `ImmutableSmallSet`?


================
Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:64
+      return llvm::find(Vector, V) == Vector.end() ? 0 : 1;
+    } else {
+      return Set.count(V);
----------------
No `else` after `return`.


================
Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:70
+
+template <typename T, unsigned SmallSize> class SmartSmallSetVector {
+public:
----------------
Same complaint here about `Smart` -- should probably just be a `SmallSetVector`.


================
Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:203
+    Decl *D = N->getDecl();
+    diag(D->getLocation(), "function '%0' is part of call graph loop")
+        << cast<NamedDecl>(D)->getName();
----------------
I think "call graph loop" is a bit much for a diagnostic -- how about `%0 is within a recursive call chain` or something along those lines?


================
Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:204
+    diag(D->getLocation(), "function '%0' is part of call graph loop")
+        << cast<NamedDecl>(D)->getName();
+  }
----------------
Drop the call to `getName()` and remove the single quotes from the diagnostic around `%0`, and just pass in the `NamedDecl` -- the diagnostics engine will do the right thing (and it gives better names for strange things like operators).


================
Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:230
+  diag(CycleEntryFn->getLocation(),
+       "Example call graph loop, starting from function '%0'",
+       DiagnosticIDs::Note)
----------------
Example -> example

I think "call graph group" might be a bit much for a diagnostic -- how about `example recursive call chain, starting from %0`


================
Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:232
+       DiagnosticIDs::Note)
+      << cast<NamedDecl>(CycleEntryFn)->getName();
+  for (int CurFrame = 1, NumFrames = CyclicCallStack.size();
----------------
Same here (and elsewhere, I'll stop commenting on them).


================
Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:249-250
+  diag(CyclicCallStack.back().CallExpr->getBeginLoc(),
+       "... which was the starting point of this call graph."
+       " This report is non-exhaustive, there may be other cycles.",
+       DiagnosticIDs::Note);
----------------
Diagnostic text should not be full sentences.

`... which was the starting point of the recursive call chain; there may be other cycles`


================
Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:259
+  CG.addToCallGraph(const_cast<TranslationUnitDecl *>(TU));
+  // CG.viewGraph();
+
----------------
Remove commented out code.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/misc-no-recursion.rst:8
+that are loops), diagnoses each function in the cycle,
+and displays one example of possible call graph loop (recursion).
----------------
Eugene.Zelenko wrote:
> It'll be reasonable to add links to relevant coding guidelines.
Agreed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72362