[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