aaron.ballman added a comment.
Thank you for this! I don't think that the check should live in `misc`. It
should have an alias in the `CERT` module, but maybe we want to have this live
in a `posix` module? If not, this should probably live in `bugprone`.
================
Comment at: clang-tools-extra/clang-tidy/misc/BadSignalToKillThreadCheck.cpp:31
+void BadSignalToKillThreadCheck::check(const MatchFinder::MatchResult &Result)
{
+ const auto IsSigterm = [](const auto &KeyValue) -> bool {
+ return KeyValue.first->getName() == "SIGTERM";
----------------
You can drop the trailing return type on the lambda.
================
Comment at: clang-tools-extra/clang-tidy/misc/BadSignalToKillThreadCheck.cpp:35
+ const auto TryExpandAsInteger =
+ [PP = PP](Preprocessor::macro_iterator It) -> Optional<unsigned> {
+ if (It == PP->macro_end())
----------------
This lambda capture looks suspicious -- why do you need to initialize the
capture?
================
Comment at: clang-tools-extra/clang-tidy/misc/BadSignalToKillThreadCheck.cpp:59
+ diag(MatchedExpr->getBeginLoc(),
+ "Thread should not be terminated by SIGTERM signal.");
+ }
----------------
Clang-tidy diagnostics are not supposed to be grammatically correct. I think a
better way to phrase it might be something like: `thread should not be
terminated by raising the 'SIGTERM' signal`
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/misc-bad-signal-to-kill-thread.rst:6
+
+Finds ``pthread_kill`` function calls when thread is terminated by
+``SIGTERM`` signal and the signal kills the entire process, not just the
----------------
when thread is terminated by -> when a thread is terminated by raising the
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/misc-bad-signal-to-kill-thread.rst:8
+``SIGTERM`` signal and the signal kills the entire process, not just the
+individual thread. Use any signal except ``SIGTERM`` or ``SIGKILL``.
+
----------------
Why does the check not look for `SIGKILL` as well as `SIGTERM`?
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69181/new/
https://reviews.llvm.org/D69181
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
