[PATCH] Implement LWG 2904 for std::variant assignment
Jonathan Wakely
jwakely@redhat.com
Tue Apr 23 23:00:00 GMT 2019
* 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.
Tested powerpc64le-linux, committed to trunk.
-------------- next part --------------
commit 4f31464fa45bfde54dfb2fb72b172797acf28c69
Author: Jonathan Wakely <jwakely@redhat.com>
Date: Tue Apr 23 23:24:46 2019 +0100
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.
diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 0c9f8a39c5c..8c7d7f37fe2 100644
--- 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 7e1b0f3bed1..a339a80d0f8 100644
--- 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();
}
More information about the Libstdc++
mailing list