aaron.ballman added a comment. This check is an interesting one. The rules around what is signal safe are changing for C++17 to be a bit more lenient than what the rules are for C++14. CERT's rule is written against C++14, and so the current behavior matches the rule wording. However, the *intent* of the rule is to ensure that only signal-safe functionality is used from a signal handler, and so from that perspective, I can imagine a user compiling for C++17 to want the relaxed rules to still comply with CERT's wording. What do you think?
================ Comment at: clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp:22 +namespace { +internal::Matcher<Decl> notExternCSignalHandler() { + return functionDecl(unless(isExternC())).bind("signal_handler"); ---------------- Since this is only needed once and is quite succinct, I'd just lower it into its usage. ================ Comment at: clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp:47 + +internal::Matcher<Decl> signalHandlerWithCallExpr() { + return functionDecl(hasDescendant(callExpr())) ---------------- Same here. ================ Comment at: clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp:52 + +internal::Matcher<Decl> allCallExpr() { + return decl(forEachDescendant(callExpr().bind("call"))); ---------------- And here. ================ Comment at: clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp:83 + const auto *Call = Match.getNodeAs<CallExpr>("call"); + const auto *Func = Call->getDirectCallee(); + if (!Func || !Func->getDefinition()) ---------------- Don't use `auto` for this one, since the type is neither complex nor spelled out in the initialization. ================ Comment at: clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp:84 + const auto *Func = Call->getDirectCallee(); + if (!Func || !Func->getDefinition()) + continue; ---------------- Can use `isDefined()` instead of `getDefinition()`. Then you can store off the `FunctionDecl *` for the definition and use it below in the call to `hasCxxRepr()`. ================ Comment at: clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp:102-103 + } + SourceLocation callLoc() { return FunctionCall; } + SourceLocation cxxRepLoc() { return CxxRepresentation; } + ---------------- These functions can be marked `const`. ================ Comment at: clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp:141 + diag(SignalHandler->getLocation(), + "use 'external C' prefix for signal handlers"); + diag(Result.Nodes.getNodeAs<DeclRefExpr>("signal_argument")->getLocation(), ---------------- 'extern \"C\"'' instead of 'external C'. I'd probably reword it to: `"signal handlers must be 'extern \"C\""` ================ Comment at: clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp:149 + diag(SingalHandler->getLocation(), + "do not use C++ representations in signal handlers"); + if (const auto *CxxStmt = Result.Nodes.getNodeAs<Stmt>("cxx_stmt")) ---------------- representations -> constructs (same below) ================ Comment at: docs/clang-tidy/checks/cert-msc54-cpp.rst:24 + extern "C" void cpp_signal_handler(int sig) { + //warning: do not use C++ representations in signal handlers + throw "error message"; ---------------- Space between // and warning; indent the comment. ================ Comment at: docs/clang-tidy/checks/cert-msc54-cpp.rst:29 + void install_cpp_signal_handler() { + if (SIG_ERR == std::signal(SIGTERM, cpp_signal_handler)) + return; ---------------- Indent the code. Repository: rL LLVM https://reviews.llvm.org/D33825 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits