This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] fix std::variant::swap for trivially-move-assignable types
- From: Jonathan Wakely <jwakely at redhat dot com>
- To: libstdc++ at gcc dot gnu dot org, gcc-patches at gcc dot gnu dot org
- Cc: Tim Shen <timshen91 at gmail dot com>, Ville Voutilainen <ville dot voutilainen at gmail dot com>
- Date: Tue, 7 Aug 2018 15:29:29 +0100
- Subject: Re: [PATCH] fix std::variant::swap for trivially-move-assignable types
- References: <20180807142403.GO25399@redhat.com>
On 07/08/18 15:24 +0100, Jonathan Wakely wrote:
This patch fixes the bug, but is it correct?
IIUC the _M_destructive_move effects don't depend on whether move
assignment is trivial, so should be defined the same way in both
specializations. It also looks like we can use it in the non-trivial
move assignment.
Should we define _M_destructive_move on _Move_ctor_base instead of
_Move_assign_base, so the duplication could be avoided?
Or maybe into _Move_ctor_base as in the attached patch. That allows it
to be used in _Copy_assign_base, and means we can omit the try-catch
block when the move construction is trivial.
diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 66d878142a4..5dd00b05f1f 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -506,6 +506,20 @@ namespace __variant
}
}
+ void _M_destructive_move(_Move_ctor_base&& __rhs)
+ {
+ this->~_Move_ctor_base();
+ __try
+ {
+ ::new (this) _Move_ctor_base(std::move(__rhs));
+ }
+ __catch (...)
+ {
+ this->_M_index = variant_npos;
+ __throw_exception_again;
+ }
+ }
+
_Move_ctor_base(const _Move_ctor_base&) = default;
_Move_ctor_base& operator=(const _Move_ctor_base&) = default;
_Move_ctor_base& operator=(_Move_ctor_base&&) = default;
@@ -516,6 +530,12 @@ namespace __variant
{
using _Base = _Copy_ctor_alias<_Types...>;
using _Base::_Base;
+
+ void _M_destructive_move(_Move_ctor_base&& __rhs)
+ {
+ this->~_Move_ctor_base();
+ ::new (this) _Move_ctor_base(std::move(__rhs));
+ }
};
template<typename... _Types>
@@ -544,16 +564,7 @@ namespace __variant
else
{
_Copy_assign_base __tmp(__rhs);
- this->~_Copy_assign_base();
- __try
- {
- ::new (this) _Copy_assign_base(std::move(__tmp));
- }
- __catch (...)
- {
- this->_M_index = variant_npos;
- __throw_exception_again;
- }
+ this->_M_destructive_move(std::move(__tmp));
}
__glibcxx_assert(this->_M_index == __rhs._M_index);
return *this;
@@ -582,20 +593,6 @@ namespace __variant
using _Base = _Copy_assign_alias<_Types...>;
using _Base::_Base;
- void _M_destructive_move(_Move_assign_base&& __rhs)
- {
- this->~_Move_assign_base();
- __try
- {
- ::new (this) _Move_assign_base(std::move(__rhs));
- }
- __catch (...)
- {
- this->_M_index = variant_npos;
- __throw_exception_again;
- }
- }
-
_Move_assign_base&
operator=(_Move_assign_base&& __rhs)
noexcept(_Traits<_Types...>::_S_nothrow_move_assign)
@@ -613,16 +610,7 @@ namespace __variant
else
{
_Move_assign_base __tmp(std::move(__rhs));
- this->~_Move_assign_base();
- __try
- {
- ::new (this) _Move_assign_base(std::move(__tmp));
- }
- __catch (...)
- {
- this->_M_index = variant_npos;
- __throw_exception_again;
- }
+ this->_M_destructive_move(std::move(__tmp));
}
__glibcxx_assert(this->_M_index == __rhs._M_index);
return *this;
@@ -638,6 +626,20 @@ namespace __variant
{
using _Base = _Copy_assign_alias<_Types...>;
using _Base::_Base;
+
+ void _M_destructive_move(_Move_assign_base&& __rhs)
+ {
+ this->~_Move_assign_base();
+ __try
+ {
+ ::new (this) _Move_assign_base(std::move(__rhs));
+ }
+ __catch (...)
+ {
+ this->_M_index = variant_npos;
+ __throw_exception_again;
+ }
+ }
};
template<typename... _Types>