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

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D113517#3122217 <https://reviews.llvm.org/D113517#3122217>, @rsmith wrote:

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

Well, "throw(X)" is not even permitted by C++17. In Clang, we allow that it only if you turn off the default-error warning option. Given that, I'm not too worried about it keeping the legacy behavior, while "throw()" gets the new behavior.



================
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();
----------------
rsmith wrote:
> 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`.
Agreed, that's better. I'll make that change and submit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113517