[PATCH] D87449: [clang-tidy] Add new check for SEI CERT rule SIG30-C

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:33
+
+  for (const FunctionDecl *D : Node.redecls())
+    if (D->getASTContext().getSourceManager().isInSystemHeader(
----------------
I'm not certain I understand why we're looking through the entire redeclaration chain to see if the function is ever mentioned in a system header. I was expecting we'd look at the expansion location of the declaration and see if that's in a system header, which is already handled by the `isExpansionInSystemHeader()` matcher. Similar below.


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:84
+      functionDecl(hasName(SignalFun), parameterCountIs(2),
+                   anyOf(isInStdNamespace(), isTopLevelSystemFunction()));
+  auto HandlerExpr =
----------------
I think this can be simplified to remove the `anyOf`:
```
functionDecl(hasAnyName("::signal", "::std::signal"), isExpansionInSystemHeader())
```
should be sufficient (if there's a function named `signal` in a system header that has the wrong parameter count, I'd be surprised).


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:97
+void SignalHandlerCheck::check(const MatchFinder::MatchResult &Result) {
+  auto *SignalCall = Result.Nodes.getNodeAs<CallExpr>("register_call");
+  auto *HandlerDecl = Result.Nodes.getNodeAs<FunctionDecl>("handler_decl");
----------------
All of these should be `const auto *` (wow, the lint pre-merge check was actually useful for once!)


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:165
+  const IdentifierInfo *II = FD->getIdentifier();
+  // Nonnamed functions are not explicitly allowed.
+  if (!II)
----------------
How about: `Unnamed functions are explicitly not allowed.`


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:171
+    return true;
+
+  return false;
----------------
I think a configuration option is needed for users to be able to add their own conforming functions and by default, that list should include the functions that POSIX specifies as being async signal safe (at least on POSIX systems, similar for Windows if Microsoft documents such a list).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87449/new/

https://reviews.llvm.org/D87449