[PATCH] D113517: Correct handling of the 'throw()' exception specifier in C++17.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

In D113517#3121455 <https://reviews.llvm.org/D113517#3121455>, @jyknight wrote:

> This change allows those future optimizations to apply to throw() as well, in C++17 mode, which is the desirable outcome.

I see. It seems inconsistent that `throw(X)` would still call `unexpected` but that `throw()` would call `terminate`, but I suppose in the `throw()` case there is really not much interesting that an `unexpected_handler` can do other than (take some logging action and) terminate the program -- in particular, it can't do exception translation. So maybe the inconsistency is not a big deal, and it's more important to get the better code generation, especially given how rare `throw(X)` is compared to `throw()`. OK, I think I'm convinced that this is the best direction.



================
Comment at: clang/lib/CodeGen/CGException.cpp:480-487
+  // In C++17 and later, 'throw()' aka EST_DynamicNone is treated the same way
+  // as noexcept. In earlier standards, it is handled separately, below.
+  if ((getLangOpts().CPlusPlus17 || EST != EST_DynamicNone) &&
+      Proto->canThrow() == CT_Cannot) {
     // noexcept functions are simple terminate scopes.
     if (!getLangOpts().EHAsynch) // -EHa: HW exception still can occur
       EHStack.pushTerminate();
----------------
Maybe the logic would be clearer if we swap the terminate and unexpected cases:
```
if  (EST == EST_Dynamic || (!getLangOpts().CPlusPlus17 && EST == EST_DynamicNone)) {
  // Prepare to call unexpected
} else if (Proto->canThrow() == CT_Cannot) {
  // Prepare to call terminate
}
```
This would keep the syntactic checks of `EST` separated from the semantic checks of `canThrow`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113517