[PATCH] D59466: [clang-tidy] openmp-exception-escape - a new check

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr accepted this revision.
gribozavr marked an inline comment as done.
gribozavr added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tidy/openmp/ExceptionEscapeCheck.cpp:53
+  Finder->addMatcher(ompExecutableDirective(
+                         unless(isStandaloneDirective()),
+                         hasStructuredBlock(stmt().bind("structured-block")))
----------------
lebedev.ri wrote:
> gribozavr wrote:
> > Do we need the `unless`?  `hasStructuredBlock()` just won't match standalone directives.
> *need*? no.
> But it makes zero sense semantically to look for structured block
> without first establishing that it has one.
> Sure, we now check that twice (here, and in `hasStructuredBlock()`), but that is up to optimizer.
> 
> The fact that `hasStructuredBlock()` workarounds the assert in
> `getStructuredBlock()` is only to avoid clang-query crashes,
> it is spelled as much in the docs.
> But it makes zero sense semantically to look for structured block without first establishing that it has one.

IDK, in my mind it makes sense.  "Does a standalone directive have a structured block?  No." is a coherent logical chain.


================
Comment at: test/clang-tidy/bugprone-exception-escape-openmp.cpp:16
+    ;
+  // FIXME: this really should be caught by bugprone-exception-escape.
+  // https://bugs.llvm.org/show_bug.cgi?id=41102
----------------
lebedev.ri wrote:
> gribozavr wrote:
> > Shouldn't the function be marked with `noexcept` for `bugprone-exception-escape` to catch it?  It does not know about OpenMP.
> > 
> > Are you suggesting that `bugprone-exception-escape` should subsume your new OpenMP-specific check?
> > Shouldn't the function be marked with noexcept for bugprone-exception-escape to catch it?
> 
> Right. I meant `noexcept` (or did i drop it and forgot to put it back?).
> But that changed nothing here, this is still an XFAIL.
> 
> > It does not know about OpenMP.
> > Are you suggesting that bugprone-exception-escape should subsume your new OpenMP-specific check?
> 
> Only the `;` is the structured-block here, so `thrower()` call *should* be caught by `bugprone-exception-escape`.
Ah, I see.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59466