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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri marked 8 inline comments as done.
lebedev.ri added a comment.

Thanks for taking a look.
Some deflective replies inline.



================
Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:21
+
+inline bool operator==(const CallGraphNode::CallRecord &LHS,
+                       const CallGraphNode::CallRecord &RHS) {
----------------
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 ?)


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


================
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;
----------------
JonasToth wrote:
> Why `smart`?
Because if it's just named `ImmutableSet`, why should it not just be a `using ImmutableSet = DenseSet; const ImmutableSet ......`?
This denotes it's different behaviour when in small-size mode and in non-small-size mode.


================
Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:73
+
+  bool isSmall() const { return Set.empty(); }
+
----------------
JonasToth wrote:
> That method name looks confusing. `isEmpty()`? If not, why?
See how it's used, e.g. in `count()`, see `SmallSet` also.


================
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();
----------------
JonasToth wrote:
> That should be a `Note:`
I'm intentionally emitting it as a warning.

If i `// NOLINT` the main warning, does that not silence the related `Notes`?
I don't want `NOLINT`ing the "main" function to silence the report for the rest of the functions in SCC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72362