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