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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.cpp:33-50
+void SignalInMultithreadedProgramCheck::registerMatchers(MatchFinder *Finder) {
+  auto signalCall =
+      callExpr(
+          ignoringImpCasts(hasDescendant(declRefExpr(hasDeclaration(
+              functionDecl(allOf(hasName("signal"), parameterCountIs(2),
+                                 hasParameter(0, hasType(isInteger())))))))))
+          .bind("signal");
----------------
abelkocsis wrote:
> steakhal wrote:
> > I apologize for interrupting the review.
> > Should we use `hasDescendant` for matching like everything in translation unit?
> > 
> > Wouldn't it be more performant to collect all the `signal` calls in a set and set a bool variable if a `ThreadList` function seen? 
> > At the end of the analysis of the translation unit, emit warnings for each recorded `signal` call if the variable was set //(aka. we are in multithreaded environment)//.
> > 
> > Treat this comment rather a question since I'm not really familiar with `clang-tidy`.
> I updated the checker not exactly in that way you mentioned, but I think you are right that we should not match for all `translationUnitDecl`.
Every single check runs on the whole `TranslationUnitDecl`, and the matcher will try to match on every of the TU's descendant, that is why it emit multiple reports in the same single run. There is no need to use `translationUnitDecl()` and nor to use `forEachDescendant()`.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.cpp:38
+                       functionDecl(hasAnyListedName(ThreadList)))))))
+              .bind("thread")),
+      hasDescendant(varDecl(hasType(recordDecl(hasName("std::thread")))))
----------------
You can emit the binding of `thread`.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.cpp:56
+  diag(MatchedSignal->getExprLoc(),
+       "singal function should not be called in a multithreaded program");
+}
----------------
`singal` -> `signal`


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D75229