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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits