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

Roman Lebedev via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Mar 22 06:28:24 PDT 2019


lebedev.ri added inline comments.


================
Comment at: clang-tidy/openmp/ExceptionEscapeCheck.cpp:53
+  Finder->addMatcher(ompExecutableDirective(
+                         unless(isStandaloneDirective()),
+                         hasStructuredBlock(stmt().bind("structured-block")))
----------------
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.


================
Comment at: docs/clang-tidy/checks/openmp-exception-escape.rst:21
+
+.. option:: IgnoredExceptions
+
----------------
gribozavr wrote:
> `IgnoredExceptionTypes`?
What do you mean?
In the check it's `Options.get("IgnoredExceptions", "")`.
That is identical to the check in bugprone.


================
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
----------------
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`.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59466





More information about the cfe-commits mailing list