steakhal added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/misc/BadSignalToKillThreadCheck.cpp:34-48 + Preprocessor::macro_iterator It = PP->macro_begin(); + while (It != PP->macro_end() && !SigtermValue.hasValue()) { + if (It->first->getName() == "SIGTERM") { + const auto *MI = PP->getMacroInfo(It->first); + const auto &T = MI->tokens().back(); + StringRef ValueStr = StringRef(T.getLiteralData(), T.getLength()); + llvm::APInt IntValue; ---------------- I think using `while` loop here makes this less readable compared to using an algorithm like `llvm::find_if`. I think a more declarative approach would help here. - Handles properly if the definition of `SIGTERM` uses other than decimal base. - Checks if `ValueStr.getAsInteger()` succeeded. Like.: ``` const auto IsSigterm = [](const auto &KeyValue) -> bool { return KeyValue.first->getName() == "SIGTERM"; }; const auto TryExpandAsInteger = [PP = PP](Preprocessor::macro_iterator It) -> Optional<unsigned> { if (It == PP->macro_end()) return llvm::None; const MacroInfo *MI = PP->getMacroInfo(It->first); const Token &T = MI->tokens().back(); StringRef ValueStr = StringRef(T.getLiteralData(), T.getLength()); llvm::APInt IntValue; constexpr unsigned AutoSenseRadix = 0; if (ValueStr.getAsInteger(AutoSenseRadix, IntValue)) return llvm::None; return IntValue.getZExtValue(); }; const auto SigtermMacro = llvm::find_if(PP->macros(), IsSigterm); if (!SigtermValue && !(SigtermValue = TryExpandAsInteger(SigtermMacro))) return; ``` ================ Comment at: clang-tools-extra/clang-tidy/misc/BadSignalToKillThreadCheck.cpp:55 + diag(MatchedExpr->getBeginLoc(), + "Thread should not be terminated by signal."); + } ---------------- I think it would be valuable to mention the name of the signal. ================ Comment at: clang-tools-extra/docs/ReleaseNotes.rst:76-77 +- New :doc:`misc-signal-terminated-thread + <clang-tidy/checks/misc-signal-terminated-thread>` check. + ---------------- Eugene.Zelenko wrote: > Please keep alphabetical order in new checks list. Your checker's name is different from these. Could you check these two lines please? ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/misc-bad-signal-to-kill-thread.rst:7 +Warn on uses of the ``pthread_kill`` function when thread is +terminated by ``SIGTERM`` signal. +.. code-block: c++ ---------------- It would be kind to offer a compliant solution for fixing the warning. ================ Comment at: clang-tools-extra/test/clang-tidy/misc-bad-signal-to-kill-thread.cpp:26 + + if ((result = pthread_cancel(thread)) != 0) { + } ---------------- Maybe worth to note here that this would be the compliant solution. Also check if it doesn't warn for different signals, but warns for the `pthread_kill(thread, 0xF))` 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