Remove the try/catch codepath if `swap` is `noexcept`.

[mpark]

Summary

In the case where swap is noexcept, we should avoid the extension to provide strong-exception guarantee.

Fixes https://bugs.llvm.org/show_bug.cgi?id=46342

[zoecarver]

Thanks for making this patch. Other than the noexcept issue, this LGTM.

libcxx/include/variant

1046

I think it would be good to also mark this noexcept(__all<(is_nothrow_move_constructible_v<_Types> && is_nothrow_swappable_v<_Types>)...>::value).

1072

I think this is OK but, are there any compilers that we support that don't support if constexpr in C++17?

[ldionne]

This LGTM. It would be great to add a test though. Would it make sense to have a test under libcxx/test/libcxx that checks the codegen like we do in several places in Clang?

libcxx/include/variant

1046

Yeah, I was going to suggest the same thing. If it makes sense. If not, why?

[miscco]

I would second @ldionne that we should add some static_assert(noexcept(...) tests

libcxx/include/variant

1065

Should that be __ugly?

[mclow.lists]

libcxx/include/variant

1046

You pretty much have to do that, otherwise you get the try/catch/terminate block in the caller

[mpark]

libcxx/include/variant

1046

I would actually prefer to not mark this noexcept, because trying to propagate noexcept down doesn't really scale.
As in, if we mark this with the noexcept condition, we'd also, with the same principle, mark __generic_construct,
__visit_alt_at, and essentially propagate it down to all of the helper functions like __assign, etc.

1072

Not that I know of...

[mpark]

Uglify is_noexcept variable.

[mpark]

@ldionne I didn't know we had a mechanism to check codegen. Could you point me to an example?

[mpark]

libcxx/include/variant

1046

Hm, I don't think this is true based on https://gcc.godbolt.org/z/68_ZWp

[ldionne]

In D81954#2096406, @mpark wrote:

@ldionne I didn't know we had a mechanism to check codegen. Could you point me to an example?

For example clang/test/CodeGenCXX/blocks-cxx11.cpp (code). This would mean adding a dependency on FileCheck, but that might be OK anyway. WDYT?

libcxx/include/variant

1046

I'm satisfied with this argument.

[zoecarver]

libcxx/include/variant

1046

Given the Godbolt link, I'm OK not marking this noexcept. But, I would still rather we mark it noexcept and, for what it's worth, I think we should probably also mark all those functions noexcept. IMO it's better to mark as many functions as possible with noexcept as long as the standard allows it (and that seems to be what we do in most places). For example, if we use __swap in another function/member internally and that function isn't marked noexcept, we have suddenly added a lot of bloat for _no reason at all_.

[ldionne]

In D81954#2096427, @ldionne wrote:

In D81954#2096406, @mpark wrote:

@ldionne I didn't know we had a mechanism to check codegen. Could you point me to an example?

For example clang/test/CodeGenCXX/blocks-cxx11.cpp (code). This would mean adding a dependency on FileCheck, but that might be OK anyway. WDYT?

Geez, I'm trying to add the ability to run FileCheck inside libc++ tests and it's a pain. Let's not block this fix on that.

[miscco]

libcxx/include/variant

1046

While I would also prefere to put noexcept on every little helper it feels out of scope for this bug fix. That is more of a larger audit of the whole thing.

[lewissbaker]

libcxx/include/variant

1070

Why is the is_nothrow_swappable_v requirement checked here?
The code below is just calling generic_construct and move_nothrow, neither of which seem to call swap().

[mpark]

libcxx/include/variant

1070

Thanks! I've followed up with https://reviews.llvm.org/D83274

libcxx/include/variant

public:
   template <size_t _Ip, class _Arg>
   inline _LIBCPP_INLINE_VISIBILITY
   void __assign(_Arg&& __arg) {
     this->__assign_alt(__access::__base::__get_alt<_Ip>(*this),
                        _VSTD::forward<_Arg>(__arg));
   }
 
   inline _LIBCPP_INLINE_VISIBILITY
   void __swap(__impl& __that)  {
     if (this->valueless_by_exception() && __that.valueless_by_exception()) {
       // do nothing.
     } else if (this->index() == __that.index()) {
       __visitation::__base::__visit_alt_at(
           this->index(),
           [](auto& __this_alt, auto& __that_alt) {
             using _VSTD::swap;
             swap(__this_alt.__value, __that_alt.__value);
           },
           *this,
           __that);
     } else {
       __impl* __lhs = this;
       __impl* __rhs = _VSTD::addressof(__that);
       if (__lhs->__move_nothrow() && !__rhs->__move_nothrow()) {
         _VSTD::swap(__lhs, __rhs);
       }
       __impl __tmp(_VSTD::move(*__rhs));
-#ifndef _LIBCPP_NO_EXCEPTIONS
+      static constexpr bool is_noexcept =
+#ifdef _LIBCPP_NO_EXCEPTIONS
+          true;
+#else
+          __all<(is_nothrow_move_constructible_v<_Types> &&
+                 is_nothrow_swappable_v<_Types>)...>::value;
+#endif
+      if constexpr (is_noexcept) {
+        this->__generic_construct(*__rhs, _VSTD::move(*__lhs));
+      } else {
         // EXTENSION: When the move construction of `__lhs` into `__rhs` throws
         // and `__tmp` is nothrow move constructible then we move `__tmp` back
         // into `__rhs` and provide the strong exception safety guarantee.
         try {
           this->__generic_construct(*__rhs, _VSTD::move(*__lhs));
         } catch (...) {
           if (__tmp.__move_nothrow()) {
             this->__generic_construct(*__rhs, _VSTD::move(__tmp));
           }
           throw;
         }
-#else
-      this->__generic_construct(*__rhs, _VSTD::move(*__lhs));
-#endif
+      }
       this->__generic_construct(*__lhs, _VSTD::move(__tmp));
     }
   }
 
 private:
   inline _LIBCPP_INLINE_VISIBILITY
   bool __move_nothrow() const {
     constexpr bool __results[] = {is_nothrow_move_constructible_v<_Types>...};