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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from some nits.



================
Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:70
+
+template <typename T, unsigned SmallSize> class SmartSmallSetVector {
+public:
----------------
lebedev.ri wrote:
> aaron.ballman wrote:
> > Same complaint here about `Smart` -- should probably just be a `SmallSetVector`.
> There already is a `SmallSetVector`, and it does have different semantics,
> i've added a code comment explaining their differences, PTAL.
> 
> For now, i'd prefer to keep this as-is, but afterwards i suspect
> i might be able to "upstream" this into `SmallSetVector` itself,
> thus getting rid of `SmartSmallSetVector`.
> 
> Let me know if this makes sense?
Okay, that's reasonable enough. Can you add a FIXME suggesting that plan in the code?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/misc-no-recursion.rst:6
+
+Finds strongly connected functions (by analyzing call graph for SCC's
+that are loops), diagnoses each function in the cycle,
----------------
call graph -> the call graph

And you should probably spell out SCC.


================
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).
+
----------------
call graph -> a call graph


================
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).
----------------
lebedev.ri wrote:
> aaron.ballman wrote:
> > Eugene.Zelenko wrote:
> > > It'll be reasonable to add links to relevant coding guidelines.
> > Agreed.
> Is this better?
Looks great!


================
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]
----------------
lebedev.ri wrote:
> JonasToth wrote:
> > 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.
> This indeed gets blinded by function pointers, test added.
> 
> As for lambdas, i'm not sure what specifically you mean?
> The lambda itself can't be recursive i think?
> https://godbolt.org/z/GYLRnB
There are techniques to make lambdas recursive, but maybe we don't need to catch those (at least not initially). e.g., https://riptutorial.com/cplusplus/example/8508/recursive-lambdas


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72362