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

[Balázs Kéri via Phabricator via cfe-commits]

balazske marked an inline comment as done.
balazske 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(
----------------
aaron.ballman wrote:
> 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.
This function is called from ` SignalHandlerCheck::check` when any function call is found. So the check for system header is needed. It was unclear to me what the "expansion location" means but it seems to work if using that expansion location and checking for system header, instead of this loop. I will update the code.


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:84
+      functionDecl(hasName(SignalFun), parameterCountIs(2),
+                   anyOf(isInStdNamespace(), isTopLevelSystemFunction()));
+  auto HandlerExpr =
----------------
aaron.ballman wrote:
> 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).
I was not aware of the possibility of using namespaces in the name, it is really more simple this way (and the `isExpansionInSystemHeader` seems to work good here).


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst:13
+(for ``signal`` there are additional conditions that are not checked).
+Every other system call is considered as non asynchronous-safe by the checker.
+
----------------
aaron.ballman wrote:
> I would document this as: `Any function that cannot be determined to be an asynchronous-safe function call is assumed to be non-asynchronous-safe by the checker, including function calls for which only the declaration of the called function is visible.`
"including function calls for which only the declaration of the called function is visible": Is this the better approach? The checker does not make warning for such functions in the current state.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449