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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
