From 86a57ce103ce429afd7a47dc19fde1bd6fa93742 Mon Sep 17 00:00:00 2001 From: Jonathan Wakely Date: Wed, 24 Apr 2019 00:01:12 +0100 Subject: [PATCH] Implement LWG 2904 for std::variant assignment * include/std/variant (__variant_construct): Use template parameter type instead of equivalent decltype-specifier. (_Move_ctor_base::_Move_ctor_base(_Move_ctor_base&&)): Replace forward with move. (_Move_ctor_base::_M_destructive_move) (_Move_ctor_base::_M_destructive_copy) (_Move_ctor_base::_M_destructive_move) (_Move_ctor_base::_M_destructive_copy): Only set the index after construction succeeds. (_Copy_assign_base::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::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 --- libstdc++-v3/ChangeLog | 23 +++++ libstdc++-v3/include/std/variant | 94 +++++++------------ .../20_util/variant/exception_safety.cc | 41 ++++++++ 3 files changed, 96 insertions(+), 62 deletions(-) diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog index a12f26e6373..27fb4fed5e3 100644 --- a/libstdc++-v3/ChangeLog +++ b/libstdc++-v3/ChangeLog @@ -1,3 +1,26 @@ +2019-04-24 Jonathan Wakely + + * include/std/variant (__variant_construct): Use template parameter + type instead of equivalent decltype-specifier. + (_Move_ctor_base::_Move_ctor_base(_Move_ctor_base&&)): + Replace forward with move. + (_Move_ctor_base::_M_destructive_move) + (_Move_ctor_base::_M_destructive_copy) + (_Move_ctor_base::_M_destructive_move) + (_Move_ctor_base::_M_destructive_copy): Only set the + index after construction succeeds. + (_Copy_assign_base::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::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 Document PSTL linker flags 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( __rhs_mem)); + std::forward(__rhs_mem)); return {}; - }, __variant_cast<_Types...>(std::forward(__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 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 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 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, - __remove_cvref_t>) - __this_mem = __rhs_mem; - } - } + __variant::__get<__rhs_index>(*this) = __rhs_mem; else { - using __rhs_type = - remove_reference_t; - 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; + 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, - remove_reference_t>) - __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>(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 v1(std::in_place_type), v2(2); + try + { + v2 = v1; // uses variant::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::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(); } -- 2.30.2