[PATCH] D75229: [clang-tidy] Add signal-in-multithreaded-program check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.cpp:41
+                hasDeclaration(functionDecl(hasAnyListedName(ThreadList)))))))),
+            hasDescendant(varDecl(hasType(recordDecl(hasName("std::thread")))))
+
----------------
`::std::thread`


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.cpp:47
+          ignoringImpCasts(hasDescendant(declRefExpr(hasDeclaration(
+              functionDecl(allOf(hasName("signal"), parameterCountIs(2),
+                                 hasParameter(0, hasType(isInteger())))))))),
----------------
`::signal`


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.cpp:56
+    const MatchFinder::MatchResult &Result) {
+  bool IsPosix = PP->isMacroDefined("__unix__") ||
+                 Result.Context->getTargetInfo().getTriple().getVendor() ==
----------------
This does not seem like the right way to check for POSIX -- isn't that `_POSIX_C_SOURCE`?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.h:24
+/// check considers the analyzed program multithreaded if it finds at least
+/// one function call of the following: ``thrd_create``, ``std::thread``,
+/// ``boost::thread``, ``pthread_t``.
----------------
The comment is a bit stale as it also accepts user-defined functions. Note, I think `CreateThread`, `CreateRemoteThread`, `_beginthread`, and `_beginthreadex` should be added to the list as those are the common Win32 APIs for creating threads (where pthreads aren't available).


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.h:34
+        ThreadList(Options.get(
+            "ThreadList", "thrd_create;::thread;boost::thread;pthread_t")) {}
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
----------------
I think that should be `::std::thread` and `::boost::thread` to avoid pathological naming issues.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D75229