[PATCH] D33825: [clang-tidy] signal handler must be plain old function check

[Aaron Ballman via Phabricator via cfe-commits]

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