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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/openmp/ExceptionEscapeCheck.cpp:32
+  llvm::StringSet<> IgnoredExceptions;
+  StringRef(RawIgnoredExceptions).split(IgnoredExceptionsVec, ",", -1, false);
+  IgnoredExceptions.insert(IgnoredExceptionsVec.begin(),
----------------
Do you need to trim any of the split strings?


================
Comment at: clang-tidy/openmp/ExceptionEscapeCheck.cpp:48
+    return;
+  // Similarly, if C++ Exceptions are not enabled, nothing to do.
+  if (!getLangOpts().CPlusPlus || !getLangOpts().CXXExceptions)
----------------
Do you have to worry about SEH exceptions in C for this functionality?


================
Comment at: clang-tidy/openmp/ExceptionEscapeCheck.cpp:72
+  // FIXME: We should provide more information about the exact location where
+  // the exception is thrown, maybe the full path the exception escapes
+
----------------
Missing a full stop at the end of the comment.


================
Comment at: clang-tidy/openmp/ExceptionEscapeCheck.cpp:75-76
+  diag(Directive->getBeginLoc(),
+       "An exception thrown inside of the OpenMP '%0' region is not caught in "
+       "that same region.")
+      << getOpenMPDirectiveName(Directive->getDirectiveKind());
----------------
Clang-tidy diagnostics are not complete sentences -- please make this horribly ungrammatical. ;-)


================
Comment at: docs/clang-tidy/checks/openmp-exception-escape.rst:9
+
+As per the OpenMP specification, structured block is an executable statement,
+possibly compound, with a single entry at the top and a single exit at the
----------------
, structured block -> , a structured block


================
Comment at: docs/clang-tidy/checks/openmp-exception-escape.rst:10-11
+As per the OpenMP specification, structured block is an executable statement,
+possibly compound, with a single entry at the top and a single exit at the
+bottom. Which means, ``throw`` may not be used to to 'exit' out of the
+structured block. If an exception is not caught in the same structured block
----------------
Does this mean setjmp/longjmp out of the block is also a dangerous activity? What about gotos?


================
Comment at: docs/clang-tidy/checks/openmp-exception-escape.rst:13
+structured block. If an exception is not caught in the same structured block
+it was thrown in, the behaviour is undefined / implementation defined,
+the program will likely terminate.
----------------
Which is it -- undefined or implementation-defined?


================
Comment at: docs/clang-tidy/checks/openmp-exception-escape.rst:23
+
+   Comma separated list containing type names which are not counted as thrown
+   exceptions in the check. Default value is an empty string.
----------------
Comma separated -> Comma-separated


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59466