Implement LWG 2904 for std::variant assignment

	* include/std/variant (__variant_construct): Use template parameter
	type instead of equivalent decltype-specifier.
	(_Move_ctor_base<false, Types...>::_Move_ctor_base(_Move_ctor_base&&)):
	Replace forward with move.
	(_Move_ctor_base<false, Types...>::_M_destructive_move)
	(_Move_ctor_base<false, Types...>::_M_destructive_copy)
	(_Move_ctor_base<true, Types...>::_M_destructive_move)
	(_Move_ctor_base<true, Types...>::_M_destructive_copy): Only set the
	index after construction succeeds.
	(_Copy_assign_base<false, Types...>::operator=): Remove redundant
	if-constexpr checks that are always true. Use __remove_cvref_t instead
	of remove_reference so that is_nothrow_move_constructible check
	doesn't use a const rvalue parameter. In the potentially-throwing case
	construct a temporary and move assign it, as per LWG 2904.
	(_Move_assign_base<false, Types...>::operator=): Remove redundant
	if-constexpr checks that are always true. Use emplace as per LWG 2904.
	(variant::operator=(T&&)): Only use emplace conditionally, otherwise
	construct a temporary and move assign from it, as per LWG 2904.
	* testsuite/20_util/variant/exception_safety.cc: Check that
	assignment operators have strong exception safety guarantee.

From-SVN: r270525

diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog
index a12f26e63736e118547080f7c035c7f1eef17ad3..27fb4fed5e3c649f218ab15490e81eb442685569 100644 (file)
--- a/libstdc++-v3/ChangeLog
+++ b/libstdc++-v3/ChangeLog
@@ -1,3 +1,26 @@
+2019-04-24  Jonathan Wakely  <jwakely@redhat.com>
+
+       * include/std/variant (__variant_construct): Use template parameter
+       type instead of equivalent decltype-specifier.
+       (_Move_ctor_base<false, Types...>::_Move_ctor_base(_Move_ctor_base&&)):
+       Replace forward with move.
+       (_Move_ctor_base<false, Types...>::_M_destructive_move)
+       (_Move_ctor_base<false, Types...>::_M_destructive_copy)
+       (_Move_ctor_base<true, Types...>::_M_destructive_move)
+       (_Move_ctor_base<true, Types...>::_M_destructive_copy): Only set the
+       index after construction succeeds.
+       (_Copy_assign_base<false, Types...>::operator=): Remove redundant
+       if-constexpr checks that are always true. Use __remove_cvref_t instead
+       of remove_reference so that is_nothrow_move_constructible check
+       doesn't use a const rvalue parameter. In the potentially-throwing case
+       construct a temporary and move assign it, as per LWG 2904.
+       (_Move_assign_base<false, Types...>::operator=): Remove redundant
+       if-constexpr checks that are always true. Use emplace as per LWG 2904.
+       (variant::operator=(T&&)): Only use emplace conditionally, otherwise
+       construct a temporary and move assign from it, as per LWG 2904.
+       * testsuite/20_util/variant/exception_safety.cc: Check that
+       assignment operators have strong exception safety guarantee.
+
 2019-04-23  Thomas Rodgers <trodgers@redhat.com>
 
        Document PSTL linker flags

diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 0c9f8a39c5c7dcb0cf400bfa820c8a06e7530bca..8c7d7f37fe28cdc9175442d9ca6a2a14b146be26 100644 (file)
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -477,9 +477,9 @@ namespace __variant
                 -> __detail::__variant::__variant_cookie
         {
          __variant_construct_single(std::forward<_Tp>(__lhs),
-             std::forward<decltype(__rhs_mem)>( __rhs_mem));
+             std::forward<decltype(__rhs_mem)>(__rhs_mem));
          return {};
-       }, __variant_cast<_Types...>(std::forward<decltype(__rhs)>(__rhs)));
+       }, __variant_cast<_Types...>(std::forward<_Up>(__rhs)));
     }
 
   // The following are (Copy|Move) (ctor|assign) layers for forwarding
@@ -522,41 +522,23 @@ namespace __variant
       _Move_ctor_base(_Move_ctor_base&& __rhs)
          noexcept(_Traits<_Types...>::_S_nothrow_move_ctor)
       {
-       __variant_construct<_Types...>(*this,
-         std::forward<_Move_ctor_base>(__rhs));
+       __variant_construct<_Types...>(*this, std::move(__rhs));
       }
 
       template<typename _Up>
         void _M_destructive_move(unsigned short __rhs_index, _Up&& __rhs)
         {
          this->_M_reset();
+         __variant_construct_single(*this, std::forward<_Up>(__rhs));
          this->_M_index = __rhs_index;
-         __try
-           {
-             __variant_construct_single(*this,
-                                        std::forward<_Up>(__rhs));
-           }
-         __catch (...)
-           {
-             this->_M_index = variant_npos;
-             __throw_exception_again;
-           }
        }
 
       template<typename _Up>
         void _M_destructive_copy(unsigned short __rhs_index, const _Up& __rhs)
         {
          this->_M_reset();
+         __variant_construct_single(*this, __rhs);
          this->_M_index = __rhs_index;
-         __try
-           {
-             __variant_construct_single(*this, __rhs);
-           }
-         __catch (...)
-           {
-             this->_M_index = variant_npos;
-             __throw_exception_again;
-           }
        }
 
       _Move_ctor_base(const _Move_ctor_base&) = default;
@@ -574,17 +556,16 @@ namespace __variant
         void _M_destructive_move(unsigned short __rhs_index, _Up&& __rhs)
         {
          this->_M_reset();
+         __variant_construct_single(*this, std::forward<_Up>(__rhs));
          this->_M_index = __rhs_index;
-         __variant_construct_single(*this,
-                                    std::forward<_Up>(__rhs));
        }
 
       template<typename _Up>
         void _M_destructive_copy(unsigned short __rhs_index, const _Up& __rhs)
         {
          this->_M_reset();
-         this->_M_index = __rhs_index;
          __variant_construct_single(*this, __rhs);
+         this->_M_index = __rhs_index;
        }
     };
 
@@ -609,31 +590,23 @@ namespace __variant
            if constexpr (__rhs_index != variant_npos)
              {
                if (this->_M_index == __rhs_index)
-                 {
-                   if constexpr (__rhs_index != variant_npos)
-                     {
-                       auto& __this_mem =
-                         __variant::__get<__rhs_index>(*this);
-                       if constexpr (is_same_v<
-                                     remove_reference_t<decltype(__this_mem)>,
-                                     __remove_cvref_t<decltype(__rhs_mem)>>)
-                         __this_mem = __rhs_mem;
-                     }
-                 }
+                 __variant::__get<__rhs_index>(*this) = __rhs_mem;
                else
                  {
-                   using __rhs_type =
-                     remove_reference_t<decltype(__rhs_mem)>;
-                   if constexpr
-                     (is_nothrow_copy_constructible_v<__rhs_type>
-                      || !is_nothrow_move_constructible_v<__rhs_type>)
-                       this->_M_destructive_copy(__rhs_index, __rhs_mem);
+                   using __rhs_type = __remove_cvref_t<decltype(__rhs_mem)>;
+                   if constexpr (is_nothrow_copy_constructible_v<__rhs_type>
+                       || !is_nothrow_move_constructible_v<__rhs_type>)
+                     // The standard says this->emplace<__rhs_type>(__rhs_mem)
+                     // should be used here, but _M_destructive_copy is
+                     // equivalent in this case. Either copy construction
+                     // doesn't throw, so _M_destructive_copy gives strong
+                     // exception safety guarantee, or both copy construction
+                     // and move construction can throw, so emplace only gives
+                     // basic exception safety anyway.
+                     this->_M_destructive_copy(__rhs_index, __rhs_mem);
                    else
-                     {
-                       auto __tmp(__rhs_mem);
-                       this->_M_destructive_move(__rhs_index,
-                                                 std::move(__tmp));
-                     }
+                     __variant_cast<_Types...>(*this)
+                       = variant<_Types...>(__rhs_mem);
                  }
              }
            else
@@ -676,20 +649,10 @@ namespace __variant
            if constexpr (__rhs_index != variant_npos)
              {
                if (this->_M_index == __rhs_index)
-                 {
-                   if constexpr (__rhs_index != variant_npos)
-                     {
-                       auto& __this_mem =
-                         __variant::__get<__rhs_index>(*this);
-                       if constexpr (is_same_v<
-                                     remove_reference_t<decltype(__this_mem)>,
-                                     remove_reference_t<decltype(__rhs_mem)>>)
-                         __this_mem = std::move(__rhs_mem);
-                     }
-                 }
+                 __variant::__get<__rhs_index>(*this) = std::move(__rhs_mem);
                else
-                 this->_M_destructive_move(__rhs_index,
-                                           std::move(__rhs_mem));
+                 __variant_cast<_Types...>(*this)
+                   .template emplace<__rhs_index>(std::move(__rhs_mem));
              }
            else
              this->_M_reset();
@@ -1395,7 +1358,14 @@ namespace __variant
          if (index() == __index)
            std::get<__index>(*this) = std::forward<_Tp>(__rhs);
          else
-           this->emplace<__index>(std::forward<_Tp>(__rhs));
+           {
+             using _Tj = __accepted_type<_Tp&&>;
+             if constexpr (is_nothrow_constructible_v<_Tj, _Tp>
+                           || !is_nothrow_move_constructible_v<_Tj>)
+               this->emplace<__index>(std::forward<_Tp>(__rhs));
+             else
+               operator=(variant(std::forward<_Tp>(__rhs)));
+           }
          return *this;
        }
 

diff --git a/libstdc++-v3/testsuite/20_util/variant/exception_safety.cc b/libstdc++-v3/testsuite/20_util/variant/exception_safety.cc
index 7e1b0f3bed1bbf58145792652ad9b341175ee376..a339a80d0f86ebdb4d127fe73d0f2f7868947984 100644 (file)
--- a/libstdc++-v3/testsuite/20_util/variant/exception_safety.cc
+++ b/libstdc++-v3/testsuite/20_util/variant/exception_safety.cc
@@ -169,10 +169,51 @@ test03()
   VERIFY( bad_emplace<std::unique_ptr<int>>(v) );
 }
 
+void
+test04()
+{
+  // LWG 2904. Make variant move-assignment more exception safe
+
+  struct ThrowOnCopy
+  {
+    ThrowOnCopy() { }
+    ThrowOnCopy(const ThrowOnCopy&) { throw 1; }
+    ThrowOnCopy& operator=(const ThrowOnCopy&) { throw "shouldn't happen"; }
+    ThrowOnCopy(ThrowOnCopy&&) noexcept { }
+  };
+
+  std::variant<int, ThrowOnCopy> v1(std::in_place_type<ThrowOnCopy>), v2(2);
+  try
+  {
+    v2 = v1; // uses variant<Types...>::operator=(const variant&)
+    VERIFY( false );
+  }
+  catch (int)
+  {
+    VERIFY( !v2.valueless_by_exception() );
+    VERIFY( v2.index() == 0 );
+    VERIFY( std::get<0>(v2) == 2 );
+  }
+
+  try
+  {
+    ThrowOnCopy toc;
+    v2 = toc; // uses variant<Types...>::operator=(T&&)
+    VERIFY( false );
+  }
+  catch (int)
+  {
+    VERIFY( !v2.valueless_by_exception() );
+    VERIFY( v2.index() == 0 );
+    VERIFY( std::get<0>(v2) == 2 );
+  }
+}
+
 int
 main()
 {
   test01();
   test02();
   test03();
+  test04();
 }