baloghadamsoftware added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:31
+static bool isSystemCall(const FunctionDecl *FD) {
+ // This check does not work with function calls in std namespace.
+ if (!FD->isGlobal() || FD->isInStdNamespace())
----------------
Why? In //C++// we have everything in `std` namespace, such as `std::signal()`,
`std::abort()` or `std::_Exit()`. In `C++` the general rule is to use them
instead of the global //C// variants.
================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:35
+ // It is assumed that the function has no other re-declaration that is not
+ // in a system header. Otherwise this may produce wrong result.
+ return FD->getASTContext().getSourceManager().isInSystemHeader(
----------------
The assumption is basically right, we do not repeat declarations from system
headers but maybe we could loop over the redeclaration chain.
================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:40
+
+static bool isAllowedSystemCall(const FunctionDecl *FD) {
+ if (!FD->getIdentifier())
----------------
The name suggests that this function checks for both //system call// and
//allowed call//. I would either rename this function to simply
`isAllowedCall()` or at least put an assertion to the beginning:
`assert(isSystemCall(FD));`.
================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:44
+ const StringRef N = FD->getName();
+ if (N == AbortFun || N == ExitFun || N == QuickExitFun || N == SignalFun)
+ return true;
----------------
Maybe you could use `IdentifierInfo` instead of string comparisons.
================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:81
+ callExpr(IsSignalFunction, HandlerAsSecondArg).bind("register_call"),
+ this);
+}
----------------
More readable would be this way:
```
const auto HandlerExpr =
declRefExpr(hasDeclaration(functionDecl().bind("handler_decl")),
unless(isExpandedFromMacro("SIG_IGN")),
unless(isExpandedFromMacro("SIG_DFL")))
.bind("handler_expr");
Finder->addMatcher(
callExpr(IsSignalFunction, hasArgument(1,
HandlerExpr)).bind("register_call"),
this);
```
================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:95
+ std::deque<std::pair<const FunctionDecl *, const Expr *>> CalledFunctions{
+ {HandlerDecl, HandlerExpr}};
+
----------------
Do we really need to store `FunctionDecl` in the map? The whole code would be
much simpler if you only store the call expression and the retrieve the callee
declaration once at the beginning of the loop body. Beside simplicity this
would also reduce the memory footprint and surely not increase the execution
time.
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst:1-17
+.. title:: clang-tidy - cert-sig30-c
+
+cert-sig30-c
+============
+
+Finds functions registered as signal handlers that call non asynchronous-safe
+functions. User functions called from the handlers are checked too, as far as
----------------
Please add at least one minimal code example. (E.g. from the tests.)
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
