[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:27
+  // Find a possible redeclaration in system header.
+  for (const FunctionDecl *D : FD->redecls())
+    if (FD->getASTContext().getSourceManager().isInSystemHeader(
----------------
```
return llvm::any_of(FD->redecls(), [](const FunctionDecl *D) {
  return D->getASTContext().getSourceManager().isInSystemHeader(D->getLocation());
});
```


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:117-118
+    FunctionCallCollector Collector{[&CalledFunctions](const CallExpr *CE) {
+      if (isa<FunctionDecl>(CE->getCalleeDecl()))
+        CalledFunctions.push_back(CE);
+    }};
----------------
For correctness, I think you need to handle more than just calls to function declarations -- for instance, this should be just as problematic:
```
void some_signal_handler(int sig) {
  []{ puts("this should not be an escape hatch for the check); }();
}
```
even though the call expression in the signal handler doesn't resolve back to a function declaration. (Similar for blocks instead of lambdas.) WDYT?


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:134
+    // At insertion we have already ensured that only function calls are there.
+    const FunctionDecl *F = cast<FunctionDecl>(FunctionCall->getCalleeDecl());
+
----------------
`const auto *`


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.cpp:52
+void test() {
+  std::signal(SIGINT, handler_abort);
+  std::signal(SIGINT, handler__Exit);
----------------
I'd also like to see a test case where the handler to `signal` call is itself not a function call:
```
std::signal(SIGINT, [](int sig) {
  puts("I did the bad thing this way"); // should be diagnosed, yes?
});
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449