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