[PATCH] D69181: [clang-tidy] Adding misc-signal-terminated-thread check

[Balázs Benics via Phabricator via cfe-commits]

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