[PATCH] D96719: [clang-tidy] Add new check 'bugprone-thread-canceltype-asynchronous' and alias 'cert-pos47-c'.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp:169
         "bugprone-terminating-continue");
+    CheckFactories.registerCheck<ThreadCanceltypeAsynchronousCheck>(
+        "bugprone-thread-canceltype-asynchronous");
----------------
I think this check is likely better surfaced in the `concurrency` module. WDYT?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/ThreadCanceltypeAsynchronousCheck.cpp:29
+
+static Preprocessor *PP;
+
----------------
Any particular reason to use a global variable here rather than making it a member of `ThreadCanceltypeAsynchronousCheck`?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/ThreadCanceltypeAsynchronousCheck.cpp:34
+  if (!PthreadCancelAsynchronousValue) {
+    const auto IsAsynchronous = [](const auto &KeyValue) -> bool {
+      return KeyValue.first->getName() == "PTHREAD_CANCEL_ASYNCHRONOUS" &&
----------------
We don't use top-level `const` on local value types (here and elsewhere in the patch).


================
Comment at: clang-tools-extra/clang-tidy/bugprone/ThreadCanceltypeAsynchronousCheck.cpp:38
+    };
+    const auto TryExpandAsInteger =
+        [](Preprocessor::macro_iterator It) -> Optional<unsigned> {
----------------
Do we know that POSIX implementations always use a simple integer here, or do we have to worry about code like:
```
#define PTHREAD_CANCEL_ASYNCHRONOUS (1 << 0)
```
or
```
#define PTHREAD_CANCEL_ASYNCHRONOUS SOME_OTHER_MACRO
```


================
Comment at: clang-tools-extra/clang-tidy/bugprone/ThreadCanceltypeAsynchronousCheck.cpp:68
+    diag(MatchedExpr->getBeginLoc(),
+         "asynchronous cancelability type should not be used");
+  }
----------------
How about: `the cancel type for a pthread should not be 'PTHREAD_CANCEL_ASYNCHRONOUS'`?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-thread-canceltype-asynchronous.rst:9
+(``PTHREAD_CANCEL_ASYNCHRONOUS``) is generally unsafe, use type
+``PTHREAD_CANCEL_DEFERRED`` instead which is the default. Even with deferred
+cancellation, a cancellation point in an asynchronous signal handler may still
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96719/new/

https://reviews.llvm.org/D96719