| Message ID | CAFk2RUYK0nF-jDA_wASDSkudhMJ6p1HwcvqT4Uo9yf_zXv4paQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] Don't revisit a variant we are already visiting. | expand |
On Thu, 28 Mar 2019 at 15:07, Ville Voutilainen <ville.voutilainen@gmail.com> wrote: > > This shaves off some unnecessary codegen. In the assignment > operators and swap, we are already visiting the rhs, so we shouldn't > visit it again to get to its guts, we already have them in-hand. > > 2019-03-28 Ville Voutilainen <ville.voutilainen@gmail.com> > > Don't revisit a variant we are already visiting. > * include/std/variant (__variant_construct_single): New. > (__variant_construct): Use it. > (_M_destructive_move): Likewise, turn into a template. > (_M_destructive_copy): Likewise. > (_Copy_assign_base::operator=): Adjust. > (_Move_assign_base::operator=): Likewise. > (swap): Likewise. Minor adjustment, use direct-init in all cases: -+ auto __tmp = std::move(__rhs_mem); ++ auto __tmp(std::move(__rhs_mem)); diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant index 3932a8a..fe96cc8 100644 --- a/libstdc++-v3/include/std/variant +++ b/libstdc++-v3/include/std/variant @@ -428,20 +428,25 @@ namespace __variant using _Variant_storage_alias = _Variant_storage<_Traits<_Types...>::_S_trivial_dtor, _Types...>; + template<typename _Tp, typename _Up> + void __variant_construct_single(_Tp&& __lhs, _Up&& __rhs_mem) + { + void* __storage = std::addressof(__lhs._M_u); + using _Type = remove_reference_t<decltype(__rhs_mem)>; + if constexpr (!is_same_v<_Type, __variant_cookie>) + ::new (__storage) + _Type(std::forward<decltype(__rhs_mem)>(__rhs_mem)); + } + template<typename... _Types, typename _Tp, typename _Up> void __variant_construct(_Tp&& __lhs, _Up&& __rhs) { __lhs._M_index = __rhs._M_index; - void* __storage = std::addressof(__lhs._M_u); - __do_visit([__storage](auto&& __rhs_mem) mutable + __do_visit([&__lhs](auto&& __rhs_mem) mutable -> __detail::__variant::__variant_cookie { - using _Type = remove_reference_t<decltype(__rhs_mem)>; - if constexpr (is_same_v<__remove_cvref_t<decltype(__rhs_mem)>, - remove_cv_t<_Type>> - && !is_same_v<_Type, __variant_cookie>) - ::new (__storage) - _Type(std::forward<decltype(__rhs_mem)>(__rhs_mem)); + __variant_construct_single(std::forward<_Tp>(__lhs), + std::forward<decltype(__rhs_mem)>(__rhs_mem)); return {}; }, __variant_cast<_Types...>(std::forward<decltype(__rhs)>(__rhs))); } @@ -490,34 +495,38 @@ namespace __variant std::forward<_Move_ctor_base>(__rhs)); } - void _M_destructive_move(_Move_ctor_base&& __rhs) - { - this->_M_reset(); - __try - { - __variant_construct<_Types...>(*this, - std::forward<_Move_ctor_base>(__rhs)); - } - __catch (...) - { - this->_M_index = variant_npos; - __throw_exception_again; - } - } + template<typename _Idx, typename _Up> + void _M_destructive_move(_Idx __rhs_index, _Up&& __rhs) + { + this->_M_reset(); + this->_M_index = __rhs_index; + __try + { + __variant_construct_single(*this, + std::forward<_Up>(__rhs)); + } + __catch (...) + { + this->_M_index = variant_npos; + __throw_exception_again; + } + } - void _M_destructive_copy(const _Move_ctor_base& __rhs) - { - this->_M_reset(); - __try - { - __variant_construct<_Types...>(*this, __rhs); - } - __catch (...) - { - this->_M_index = variant_npos; - __throw_exception_again; - } - } + template<typename _Idx, typename _Up> + void _M_destructive_copy(_Idx __rhs_index, const _Up& __rhs) + { + this->_M_reset(); + 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; _Move_ctor_base& operator=(const _Move_ctor_base&) = default; @@ -530,17 +539,21 @@ namespace __variant using _Base = _Copy_ctor_alias<_Types...>; using _Base::_Base; - void _M_destructive_move(_Move_ctor_base&& __rhs) - { - this->_M_reset(); - __variant_construct<_Types...>(*this, - std::forward<_Move_ctor_base>(__rhs)); - } - void _M_destructive_copy(const _Move_ctor_base& __rhs) - { - this->_M_reset(); - __variant_construct<_Types...>(*this, __rhs); - } + template<typename _Idx, typename _Up> + void _M_destructive_move(_Idx __rhs_index, _Up&& __rhs) + { + this->_M_reset(); + this->_M_index = __rhs_index; + __variant_construct_single(*this, + std::forward<_Up>(__rhs)); + } + template<typename _Idx, typename _Up> + void _M_destructive_copy(_Idx __rhs_index, const _Up& __rhs) + { + this->_M_reset(); + this->_M_index = __rhs_index; + __variant_construct_single(*this, __rhs); + } }; template<typename... _Types> @@ -580,12 +593,13 @@ namespace __variant if constexpr (is_nothrow_copy_constructible_v<__rhs_type> || !is_nothrow_move_constructible_v<__rhs_type>) { - this->_M_destructive_copy(__rhs); + this->_M_destructive_copy(__rhs._M_index, __rhs_mem); } else { - _Copy_assign_base __tmp(__rhs); - this->_M_destructive_move(std::move(__tmp)); + auto __tmp(__rhs_mem); + this->_M_destructive_move(__rhs._M_index, + std::move(__tmp)); } } else @@ -641,8 +655,8 @@ namespace __variant } else { - _Move_assign_base __tmp(std::move(__rhs)); - this->_M_destructive_move(std::move(__tmp)); + auto __tmp(std::move(__rhs_mem)); + this->_M_destructive_move(__rhs._M_index, std::move(__tmp)); } return {}; }, __variant_cast<_Types...>(*this), __variant_cast<_Types...>(__rhs)); @@ -1409,21 +1423,26 @@ namespace __variant remove_reference_t<decltype(__this_mem)>, __detail::__variant::__variant_cookie>) { - this->_M_destructive_move(std::move(__rhs)); + this->_M_destructive_move(__rhs.index(), + std::move(__rhs_mem)); __rhs._M_reset(); } else if constexpr (is_same_v< remove_reference_t<decltype(__rhs_mem)>, __detail::__variant::__variant_cookie>) { - __rhs._M_destructive_move(std::move(*this)); + __rhs._M_destructive_move(this->index(), + std::move(__this_mem)); this->_M_reset(); } else { - auto __tmp = std::move(__rhs); - __rhs._M_destructive_move(std::move(*this)); - this->_M_destructive_move(std::move(__tmp)); + auto __tmp(std::move(__rhs_mem)); + auto __idx = __rhs.index(); + __rhs._M_destructive_move(this->index(), + std::move(__this_mem)); + this->_M_destructive_move(__idx, + std::move(__tmp)); } } return {};
On 28/03/19 15:21 +0200, Ville Voutilainen wrote: >On Thu, 28 Mar 2019 at 15:07, Ville Voutilainen ><ville.voutilainen@gmail.com> wrote: >> >> This shaves off some unnecessary codegen. In the assignment >> operators and swap, we are already visiting the rhs, so we shouldn't >> visit it again to get to its guts, we already have them in-hand. >> >> 2019-03-28 Ville Voutilainen <ville.voutilainen@gmail.com> >> >> Don't revisit a variant we are already visiting. >> * include/std/variant (__variant_construct_single): New. >> (__variant_construct): Use it. >> (_M_destructive_move): Likewise, turn into a template. >> (_M_destructive_copy): Likewise. >> (_Copy_assign_base::operator=): Adjust. >> (_Move_assign_base::operator=): Likewise. >> (swap): Likewise. > >Minor adjustment, use direct-init in all cases: > >-+ auto __tmp = std::move(__rhs_mem); >++ auto __tmp(std::move(__rhs_mem)); >diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant >index 3932a8a..fe96cc8 100644 >--- a/libstdc++-v3/include/std/variant >+++ b/libstdc++-v3/include/std/variant >@@ -428,20 +428,25 @@ namespace __variant > using _Variant_storage_alias = > _Variant_storage<_Traits<_Types...>::_S_trivial_dtor, _Types...>; > >+ template<typename _Tp, typename _Up> >+ void __variant_construct_single(_Tp&& __lhs, _Up&& __rhs_mem) Please indent this line to match the opening brace, not the template-head. >@@ -490,34 +495,38 @@ namespace __variant > std::forward<_Move_ctor_base>(__rhs)); > } > >- void _M_destructive_move(_Move_ctor_base&& __rhs) >- { >- this->_M_reset(); >- __try >- { >- __variant_construct<_Types...>(*this, >- std::forward<_Move_ctor_base>(__rhs)); >- } >- __catch (...) >- { >- this->_M_index = variant_npos; >- __throw_exception_again; >- } >- } >+ template<typename _Idx, typename _Up> >+ void _M_destructive_move(_Idx __rhs_index, _Up&& __rhs) Does __rhs_index need to be a template parameter in these function templates? It seems to always be passed size_t, but the value will always fit in unsigned short. Could it just use size_t? Or even unsigned, since we don't really need to pass 64 bits? I think typename _Base::__index_type is strictly correct, but that's a lot to type and to read. OK with the indentation tweak, and I'll leave changing the _Idx or not to your judgment.
diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant index 3932a8a..003cc15 100644 --- a/libstdc++-v3/include/std/variant +++ b/libstdc++-v3/include/std/variant @@ -428,20 +428,25 @@ namespace __variant using _Variant_storage_alias = _Variant_storage<_Traits<_Types...>::_S_trivial_dtor, _Types...>; + template<typename _Tp, typename _Up> + void __variant_construct_single(_Tp&& __lhs, _Up&& __rhs_mem) + { + void* __storage = std::addressof(__lhs._M_u); + using _Type = remove_reference_t<decltype(__rhs_mem)>; + if constexpr (!is_same_v<_Type, __variant_cookie>) + ::new (__storage) + _Type(std::forward<decltype(__rhs_mem)>(__rhs_mem)); + } + template<typename... _Types, typename _Tp, typename _Up> void __variant_construct(_Tp&& __lhs, _Up&& __rhs) { __lhs._M_index = __rhs._M_index; - void* __storage = std::addressof(__lhs._M_u); - __do_visit([__storage](auto&& __rhs_mem) mutable + __do_visit([&__lhs](auto&& __rhs_mem) mutable -> __detail::__variant::__variant_cookie { - using _Type = remove_reference_t<decltype(__rhs_mem)>; - if constexpr (is_same_v<__remove_cvref_t<decltype(__rhs_mem)>, - remove_cv_t<_Type>> - && !is_same_v<_Type, __variant_cookie>) - ::new (__storage) - _Type(std::forward<decltype(__rhs_mem)>(__rhs_mem)); + __variant_construct_single(std::forward<_Tp>(__lhs), + std::forward<decltype(__rhs_mem)>(__rhs_mem)); return {}; }, __variant_cast<_Types...>(std::forward<decltype(__rhs)>(__rhs))); } @@ -490,34 +495,38 @@ namespace __variant std::forward<_Move_ctor_base>(__rhs)); } - void _M_destructive_move(_Move_ctor_base&& __rhs) - { - this->_M_reset(); - __try - { - __variant_construct<_Types...>(*this, - std::forward<_Move_ctor_base>(__rhs)); - } - __catch (...) - { - this->_M_index = variant_npos; - __throw_exception_again; - } - } + template<typename _Idx, typename _Up> + void _M_destructive_move(_Idx __rhs_index, _Up&& __rhs) + { + this->_M_reset(); + this->_M_index = __rhs_index; + __try + { + __variant_construct_single(*this, + std::forward<_Up>(__rhs)); + } + __catch (...) + { + this->_M_index = variant_npos; + __throw_exception_again; + } + } - void _M_destructive_copy(const _Move_ctor_base& __rhs) - { - this->_M_reset(); - __try - { - __variant_construct<_Types...>(*this, __rhs); - } - __catch (...) - { - this->_M_index = variant_npos; - __throw_exception_again; - } - } + template<typename _Idx, typename _Up> + void _M_destructive_copy(_Idx __rhs_index, const _Up& __rhs) + { + this->_M_reset(); + 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; _Move_ctor_base& operator=(const _Move_ctor_base&) = default; @@ -530,17 +539,21 @@ namespace __variant using _Base = _Copy_ctor_alias<_Types...>; using _Base::_Base; - void _M_destructive_move(_Move_ctor_base&& __rhs) - { - this->_M_reset(); - __variant_construct<_Types...>(*this, - std::forward<_Move_ctor_base>(__rhs)); - } - void _M_destructive_copy(const _Move_ctor_base& __rhs) - { - this->_M_reset(); - __variant_construct<_Types...>(*this, __rhs); - } + template<typename _Idx, typename _Up> + void _M_destructive_move(_Idx __rhs_index, _Up&& __rhs) + { + this->_M_reset(); + this->_M_index = __rhs_index; + __variant_construct_single(*this, + std::forward<_Up>(__rhs)); + } + template<typename _Idx, typename _Up> + void _M_destructive_copy(_Idx __rhs_index, const _Up& __rhs) + { + this->_M_reset(); + this->_M_index = __rhs_index; + __variant_construct_single(*this, __rhs); + } }; template<typename... _Types> @@ -580,12 +593,13 @@ namespace __variant if constexpr (is_nothrow_copy_constructible_v<__rhs_type> || !is_nothrow_move_constructible_v<__rhs_type>) { - this->_M_destructive_copy(__rhs); + this->_M_destructive_copy(__rhs._M_index, __rhs_mem); } else { - _Copy_assign_base __tmp(__rhs); - this->_M_destructive_move(std::move(__tmp)); + auto __tmp(__rhs_mem); + this->_M_destructive_move(__rhs._M_index, + std::move(__tmp)); } } else @@ -641,8 +655,8 @@ namespace __variant } else { - _Move_assign_base __tmp(std::move(__rhs)); - this->_M_destructive_move(std::move(__tmp)); + auto __tmp(std::move(__rhs_mem)); + this->_M_destructive_move(__rhs._M_index, std::move(__tmp)); } return {}; }, __variant_cast<_Types...>(*this), __variant_cast<_Types...>(__rhs)); @@ -1409,21 +1423,26 @@ namespace __variant remove_reference_t<decltype(__this_mem)>, __detail::__variant::__variant_cookie>) { - this->_M_destructive_move(std::move(__rhs)); + this->_M_destructive_move(__rhs.index(), + std::move(__rhs_mem)); __rhs._M_reset(); } else if constexpr (is_same_v< remove_reference_t<decltype(__rhs_mem)>, __detail::__variant::__variant_cookie>) { - __rhs._M_destructive_move(std::move(*this)); + __rhs._M_destructive_move(this->index(), + std::move(__this_mem)); this->_M_reset(); } else { - auto __tmp = std::move(__rhs); - __rhs._M_destructive_move(std::move(*this)); - this->_M_destructive_move(std::move(__tmp)); + auto __tmp = std::move(__rhs_mem); + auto __idx = __rhs.index(); + __rhs._M_destructive_move(this->index(), + std::move(__this_mem)); + this->_M_destructive_move(__idx, + std::move(__tmp)); } } return {};
This shaves off some unnecessary codegen. In the assignment operators and swap, we are already visiting the rhs, so we shouldn't visit it again to get to its guts, we already have them in-hand. 2019-03-28 Ville Voutilainen <ville.voutilainen@gmail.com> Don't revisit a variant we are already visiting. * include/std/variant (__variant_construct_single): New. (__variant_construct): Use it. (_M_destructive_move): Likewise, turn into a template. (_M_destructive_copy): Likewise. (_Copy_assign_base::operator=): Adjust. (_Move_assign_base::operator=): Likewise. (swap): Likewise.