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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
