steakhal added a comment. Unfortunately, I'm not qualified enough to have much to say.
================ Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:1 +//===--- Env32CCheck.cpp - clang-tidy -------------------------------------===// +// ---------------- Env32CCheck.cpp -> ExitHandlerCheck.cpp ================ Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:38 +bool isExitFunction(StringRef FnName) { + return FnName == EF__Exit || FnName == EF_exit || FnName == EF_quick_exit; +} ---------------- If `EF__Exit`, `EF_exit` and `EF_quick_exit` referred only once, we could simply inline those, Same for the `isJumpFunction`. ================ Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:60 + /// Iteration over the collector is iteration over the found FunctionDecls. + auto begin() const -> decltype(CalledFunctions.begin()) { + return CalledFunctions.begin(); ---------------- IMO `-> decltype(CalledFunctions.begin())` is unnecessary. Return type deduction does the right thing for this case. Same for the `auto end() ...` ================ Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:84 + hasArgument( + 0, // the first argument is the handler function + declRefExpr( ---------------- If you disabled clang-format for having inline comments, Then you could create smaller matchers and give names for them. Something similar to this: ```lang=cpp const auto DesciptiveName = hasArgument(0, declRefExpr(hasDeclaration(functionDecl().bind("handler_decl")))); ``` ================ Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:132 + "terminate by returning"); + break; + } ---------------- Why don't we `return` here? Same for the next `break`. ================ Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:148-154 + // Collect all the referenced FunctionDecls. + Collector.TraverseStmt(CurrentDefWithBody->getBody()); + // Add called functions to the worklist. + std::copy(Collector.begin(), Collector.end(), + std::back_inserter(CalledFunctions)); + // Reset the ASTVisitor instance results. + Collector.clear(); ---------------- nit: I would clear the `Collector` before the `TraverseStmt`. ================ Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h:20 +/// Checker for SEI CERT rule ENV32-C +/// All exit handlers must return normal. +/// Exit handlers must terminate by returning. It is important and potentially ---------------- In the documentation you use the: clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst > All exit handlers must return normally. You should be consistent. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83717/new/ https://reviews.llvm.org/D83717 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits