* [Patch] Forward triviality in variant @ 2017-05-30 6:36 Tim Shen via gcc-patches 2017-05-30 9:41 ` Tim Shen via gcc-patches 0 siblings, 1 reply; 11+ messages in thread From: Tim Shen via gcc-patches @ 2017-05-30 6:36 UTC (permalink / raw) To: libstdc++, gcc-patches [-- Attachment #1: Type: text/plain, Size: 802 bytes --] This patch implements <https://lichray.github.io/trivially_variant.html>, but with more changes than the proposal's. It 1) Creates __detail::__variant::_Traits as a centralized place to hold common (but not all yet) compile-time conditions. 2) Changes the noexcept conditions for the (copy|move) (ctor|assign) SMFs, so that when one is trivial, one is also noexcept. It's not the same as p0088r3, nor p0088r3 + D0602R1 anymore. 3) Creates 4 structs, namely (_Copy|_Move)_(ctor|assign)_(base|alias) for dispatch on triviality. The code that were originally in _Variant_base are moved into these four structs. There aren't functional changes except for more triviality. Sorry for having a large patch. Do tell me if you want me to split it. Tested on x86_64-linux-gnu. Thanks! -- Regards, Tim Shen [-- Attachment #2: a.diff --] [-- Type: text/x-patch, Size: 23828 bytes --] commit a4db7d21c6e4223300861114931eb0ef78bef1a6 Author: Tim Shen <timshen@google.com> Date: Mon May 29 22:44:42 2017 -0700 2017-05-30 Tim Shen <timshen@google.com> PR libstdc++/80187 * include/std/variant (variant::variant, variant::~variant, variant::operator=): Implement triviality forwarding for four special member functions. * testsuite/20_util/variant/compile.cc: Tests. diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant index b9824a5182c..8736fcc75bc 100644 --- a/libstdc++-v3/include/std/variant +++ b/libstdc++-v3/include/std/variant @@ -290,6 +290,49 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __ref_cast<_Tp>(__t)); } + template<typename... _Types> + struct _Traits + { + static constexpr bool is_default_constructible_v = + is_default_constructible_v<typename _Nth_type<0, _Types...>::type>; + static constexpr bool is_copy_constructible_v = + __and_<is_copy_constructible<_Types>...>::value; + static constexpr bool is_move_constructible_v = + __and_<is_move_constructible<_Types>...>::value; + static constexpr bool is_copy_assignable_v = + is_copy_constructible_v && is_move_constructible_v + && __and_<is_copy_assignable<_Types>...>::value; + static constexpr bool is_move_assignable_v = + is_move_constructible_v + && __and_<is_move_assignable<_Types>...>::value; + + static constexpr bool is_copy_ctor_trivial = + __and_<is_trivially_copy_constructible<_Types>...>::value; + static constexpr bool is_move_ctor_trivial = + __and_<is_trivially_move_constructible<_Types>...>::value; + static constexpr bool is_copy_assign_trivial = + __and_<is_trivially_copy_assignable<_Types>...>::value; + static constexpr bool is_move_assign_trivial = + __and_<is_trivially_move_assignable<_Types>...>::value; + static constexpr bool is_dtor_trivial = + __and_<is_trivially_destructible<_Types>...>::value; + + static constexpr bool is_default_ctor_noexcept = + is_nothrow_default_constructible_v< + typename _Nth_type<0, _Types...>::type>; + static constexpr bool is_copy_ctor_noexcept = + is_copy_ctor_trivial; + static constexpr bool is_move_ctor_noexcept = + is_move_ctor_trivial + || __and_<is_nothrow_move_constructible<_Types>...>::value; + static constexpr bool is_copy_assign_noexcept = + is_copy_assign_trivial; + static constexpr bool is_move_assign_noexcept = + is_move_assign_trivial || + (is_move_ctor_noexcept + && __and_<is_nothrow_move_assignable<_Types>...>::value); + }; + // Defines members and ctors. template<typename... _Types> union _Variadic_union { }; @@ -355,6 +398,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION ~_Variant_storage() { _M_reset(); } + void* + _M_storage() const + { + return const_cast<void*>(static_cast<const void*>( + std::addressof(_M_u))); + } + + constexpr bool + _M_valid() const noexcept + { + return this->_M_index != __index_type(variant_npos); + } + _Variadic_union<_Types...> _M_u; using __index_type = __select_index<_Types...>; __index_type _M_index; @@ -374,59 +430,114 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION void _M_reset() { _M_index = variant_npos; } + void* + _M_storage() const + { + return const_cast<void*>(static_cast<const void*>( + std::addressof(_M_u))); + } + + constexpr bool + _M_valid() const noexcept + { + return this->_M_index != __index_type(variant_npos); + } + _Variadic_union<_Types...> _M_u; using __index_type = __select_index<_Types...>; __index_type _M_index; }; - // Helps SFINAE on special member functions. Otherwise it can live in variant - // class. template<typename... _Types> - struct _Variant_base : - _Variant_storage<(std::is_trivially_destructible_v<_Types> && ...), - _Types...> - { - using _Storage = - _Variant_storage<(std::is_trivially_destructible_v<_Types> && ...), - _Types...>; + using _Variant_storage_alias = + _Variant_storage<_Traits<_Types...>::is_dtor_trivial, _Types...>; - constexpr - _Variant_base() - noexcept(is_nothrow_default_constructible_v< - variant_alternative_t<0, variant<_Types...>>>) - : _Variant_base(in_place_index<0>) { } + // The following are (Copy|Move) (ctor|assign) layers for forwarding + // triviality and handling non-trivial SMF behaviors. - _Variant_base(const _Variant_base& __rhs) + template<bool, typename... _Types> + struct _Copy_ctor_base : _Variant_storage_alias<_Types...> + { + using _Base = _Variant_storage_alias<_Types...>; + using _Base::_Base; + + _Copy_ctor_base(const _Copy_ctor_base& __rhs) + noexcept(_Traits<_Types...>::is_copy_ctor_noexcept) { if (__rhs._M_valid()) { static constexpr void (*_S_vtable[])(void*, void*) = { &__erased_ctor<_Types&, const _Types&>... }; - _S_vtable[__rhs._M_index](_M_storage(), __rhs._M_storage()); + _S_vtable[__rhs._M_index](this->_M_storage(), __rhs._M_storage()); this->_M_index = __rhs._M_index; } } - _Variant_base(_Variant_base&& __rhs) - noexcept((is_nothrow_move_constructible_v<_Types> && ...)) + _Copy_ctor_base(_Copy_ctor_base&&) + noexcept(_Traits<_Types...>::is_move_ctor_noexcept) = default; + _Copy_ctor_base& operator=(const _Copy_ctor_base&) + noexcept(_Traits<_Types...>::is_copy_assign_noexcept) = default; + _Copy_ctor_base& operator=(_Copy_ctor_base&&) + noexcept(_Traits<_Types...>::is_move_assign_noexcept) = default; + }; + + template<typename... _Types> + struct _Copy_ctor_base<true, _Types...> : _Variant_storage_alias<_Types...> + { + using _Base = _Variant_storage_alias<_Types...>; + using _Base::_Base; + }; + + template<typename... _Types> + using _Copy_ctor_alias = + _Copy_ctor_base<_Traits<_Types...>::is_copy_ctor_trivial, _Types...>; + + template<bool, typename... _Types> + struct _Move_ctor_base : _Copy_ctor_alias<_Types...> + { + using _Base = _Copy_ctor_alias<_Types...>; + using _Base::_Base; + + _Move_ctor_base(_Move_ctor_base&& __rhs) + noexcept(_Traits<_Types...>::is_move_ctor_noexcept) { if (__rhs._M_valid()) { static constexpr void (*_S_vtable[])(void*, void*) = { &__erased_ctor<_Types&, _Types&&>... }; - _S_vtable[__rhs._M_index](_M_storage(), __rhs._M_storage()); + _S_vtable[__rhs._M_index](this->_M_storage(), __rhs._M_storage()); this->_M_index = __rhs._M_index; } } - template<size_t _Np, typename... _Args> - constexpr explicit - _Variant_base(in_place_index_t<_Np> __i, _Args&&... __args) - : _Storage(__i, std::forward<_Args>(__args)...) - { } + _Move_ctor_base(const _Move_ctor_base& __rhs) + noexcept(_Traits<_Types...>::is_copy_ctor_noexcept) = default; + _Move_ctor_base& operator=(const _Move_ctor_base&) + noexcept(_Traits<_Types...>::is_copy_assign_noexcept) = default; + _Move_ctor_base& operator=(_Move_ctor_base&&) + noexcept(_Traits<_Types...>::is_move_assign_noexcept) = default; + }; + + template<typename... _Types> + struct _Move_ctor_base<true, _Types...> : _Copy_ctor_alias<_Types...> + { + using _Base = _Copy_ctor_alias<_Types...>; + using _Base::_Base; + }; + + template<typename... _Types> + using _Move_ctor_alias = + _Move_ctor_base<_Traits<_Types...>::is_move_ctor_trivial, _Types...>; + + template<bool, typename... _Types> + struct _Copy_assign_base : _Move_ctor_alias<_Types...> + { + using _Base = _Move_ctor_alias<_Types...>; + using _Base::_Base; - _Variant_base& - operator=(const _Variant_base& __rhs) + _Copy_assign_base& + operator=(const _Copy_assign_base& __rhs) + noexcept(_Traits<_Types...>::is_copy_assign_noexcept) { if (this->_M_index == __rhs._M_index) { @@ -434,16 +545,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { static constexpr void (*_S_vtable[])(void*, void*) = { &__erased_assign<_Types&, const _Types&>... }; - _S_vtable[__rhs._M_index](_M_storage(), __rhs._M_storage()); + _S_vtable[__rhs._M_index](this->_M_storage(), __rhs._M_storage()); } } else { - _Variant_base __tmp(__rhs); - this->~_Variant_base(); + _Copy_assign_base __tmp(__rhs); + this->~_Copy_assign_base(); __try { - ::new (this) _Variant_base(std::move(__tmp)); + ::new (this) _Copy_assign_base(std::move(__tmp)); } __catch (...) { @@ -455,12 +566,38 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION return *this; } - void _M_destructive_move(_Variant_base&& __rhs) + _Copy_assign_base(const _Copy_assign_base& __rhs) + noexcept(_Traits<_Types...>::is_copy_ctor_noexcept) = default; + _Copy_assign_base(_Copy_assign_base&&) + noexcept(_Traits<_Types...>::is_move_ctor_noexcept) = default; + _Copy_assign_base& operator=(_Copy_assign_base&&) + noexcept(_Traits<_Types...>::is_move_assign_noexcept) = default; + }; + + template<typename... _Types> + struct _Copy_assign_base<true, _Types...> : _Move_ctor_alias<_Types...> + { + using _Base = _Move_ctor_alias<_Types...>; + using _Base::_Base; + }; + + template<typename... _Types> + using _Copy_assign_alias = + _Copy_assign_base<_Traits<_Types...>::is_copy_assign_trivial, + _Types...>; + + template<bool, typename... _Types> + struct _Move_assign_base : _Copy_assign_alias<_Types...> + { + using _Base = _Copy_assign_alias<_Types...>; + using _Base::_Base; + + void _M_destructive_move(_Move_assign_base&& __rhs) { - this->~_Variant_base(); + this->~_Move_assign_base(); __try { - ::new (this) _Variant_base(std::move(__rhs)); + ::new (this) _Move_assign_base(std::move(__rhs)); } __catch (...) { @@ -469,40 +606,81 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } } - _Variant_base& - operator=(_Variant_base&& __rhs) - noexcept((is_nothrow_move_constructible_v<_Types> && ...) - && (is_nothrow_move_assignable_v<_Types> && ...)) + _Move_assign_base& + operator=(_Move_assign_base&& __rhs) + noexcept(_Traits<_Types...>::is_move_assign_noexcept) { if (this->_M_index == __rhs._M_index) { if (__rhs._M_valid()) { static constexpr void (*_S_vtable[])(void*, void*) = - { &__erased_assign<_Types&, _Types&&>... }; - _S_vtable[__rhs._M_index](_M_storage(), __rhs._M_storage()); + { &__erased_assign<_Types&, const _Types&>... }; + _S_vtable[__rhs._M_index](this->_M_storage(), __rhs._M_storage()); } } else { - _M_destructive_move(std::move(__rhs)); + _Move_assign_base __tmp(__rhs); + this->~_Move_assign_base(); + __try + { + ::new (this) _Move_assign_base(std::move(__tmp)); + } + __catch (...) + { + this->_M_index = variant_npos; + __throw_exception_again; + } } + __glibcxx_assert(this->_M_index == __rhs._M_index); return *this; } - void* - _M_storage() const - { - return const_cast<void*>(static_cast<const void*>( - std::addressof(_Storage::_M_u))); - } + _Move_assign_base(const _Move_assign_base& __rhs) + noexcept(_Traits<_Types...>::is_copy_ctor_noexcept) = default; + _Move_assign_base(_Move_assign_base&&) + noexcept(_Traits<_Types...>::is_move_ctor_noexcept) = default; + _Move_assign_base& operator=(const _Move_assign_base&) + noexcept(_Traits<_Types...>::is_copy_assign_noexcept) = default; + }; - constexpr bool - _M_valid() const noexcept - { - return this->_M_index != - typename _Storage::__index_type(variant_npos); - } + template<typename... _Types> + struct _Move_assign_base<true, _Types...> : _Copy_assign_alias<_Types...> + { + using _Base = _Copy_assign_alias<_Types...>; + using _Base::_Base; + }; + + template<typename... _Types> + using _Move_assign_alias = + _Move_assign_base<_Traits<_Types...>::is_move_assign_trivial, + _Types...>; + + template<typename... _Types> + struct _Variant_base : _Move_assign_alias<_Types...> + { + using _Base = _Move_assign_alias<_Types...>; + + constexpr + _Variant_base() + noexcept(_Traits<_Types...>::is_default_ctor_noexcept) + : _Variant_base(in_place_index<0>) { } + + template<size_t _Np, typename... _Args> + constexpr explicit + _Variant_base(in_place_index_t<_Np> __i, _Args&&... __args) + : _Base(__i, std::forward<_Args>(__args)...) + { } + + _Variant_base(const _Variant_base& __rhs) + noexcept(_Traits<_Types...>::is_copy_ctor_noexcept) = default; + _Variant_base(_Variant_base&&) + noexcept(_Traits<_Types...>::is_move_ctor_noexcept) = default; + _Variant_base& operator=(const _Variant_base&) + noexcept(_Traits<_Types...>::is_copy_assign_noexcept) = default; + _Variant_base& operator=(_Variant_base&&) + noexcept(_Traits<_Types...>::is_move_assign_noexcept) = default; }; // For how many times does _Tp appear in _Tuple? @@ -877,16 +1055,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION class variant : private __detail::__variant::_Variant_base<_Types...>, private _Enable_default_constructor< - is_default_constructible_v< - variant_alternative_t<0, variant<_Types...>>>, variant<_Types...>>, + __detail::__variant::_Traits<_Types...>::is_default_constructible_v, + variant<_Types...>>, private _Enable_copy_move< - (is_copy_constructible_v<_Types> && ...), - (is_copy_constructible_v<_Types> && ...) - && (is_move_constructible_v<_Types> && ...) - && (is_copy_assignable_v<_Types> && ...), - (is_move_constructible_v<_Types> && ...), - (is_move_constructible_v<_Types> && ...) - && (is_move_assignable_v<_Types> && ...), + __detail::__variant::_Traits<_Types...>::is_copy_constructible_v, + __detail::__variant::_Traits<_Types...>::is_copy_assignable_v, + __detail::__variant::_Traits<_Types...>::is_move_constructible_v, + __detail::__variant::_Traits<_Types...>::is_move_assignable_v, variant<_Types...>> { private: @@ -899,9 +1074,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION using _Base = __detail::__variant::_Variant_base<_Types...>; using _Default_ctor_enabler = - _Enable_default_constructor< - is_default_constructible_v< - variant_alternative_t<0, variant<_Types...>>>, variant<_Types...>>; + _Enable_default_constructor< + __detail::__variant::_Traits<_Types...>::is_default_constructible_v, + variant<_Types...>>; template<typename _Tp> static constexpr bool @@ -928,12 +1103,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION static constexpr size_t __index_of = __detail::__variant::__index_of_v<_Tp, _Types...>; + using _Traits = __detail::__variant::_Traits<_Types...>; + public: - constexpr variant() - noexcept(is_nothrow_default_constructible_v<__to_type<0>>) = default; - variant(const variant&) = default; + variant() noexcept(_Traits::is_default_ctor_noexcept) = default; + variant(const variant& __rhs) + noexcept(_Traits::is_copy_ctor_noexcept) = default; variant(variant&&) - noexcept((is_nothrow_move_constructible_v<_Types> && ...)) = default; + noexcept(_Traits::is_move_ctor_noexcept) = default; + variant& operator=(const variant&) + noexcept(_Traits::is_copy_assign_noexcept) = default; + variant& operator=(variant&&) + noexcept(_Traits::is_move_assign_noexcept) = default; + ~variant() = default; template<typename _Tp, typename = enable_if_t<!is_same_v<decay_t<_Tp>, variant>>, @@ -942,7 +1124,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION constexpr variant(_Tp&& __t) noexcept(is_nothrow_constructible_v<__accepted_type<_Tp&&>, _Tp&&>) - : variant(in_place_index<__accepted_index<_Tp&&>>, std::forward<_Tp>(__t)) + : variant(in_place_index<__accepted_index<_Tp&&>>, + std::forward<_Tp>(__t)) { __glibcxx_assert(holds_alternative<__accepted_type<_Tp&&>>(*this)); } template<typename _Tp, typename... _Args, @@ -950,7 +1133,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION && is_constructible_v<_Tp, _Args&&...>>> constexpr explicit variant(in_place_type_t<_Tp>, _Args&&... __args) - : variant(in_place_index<__index_of<_Tp>>, std::forward<_Args>(__args)...) + : variant(in_place_index<__index_of<_Tp>>, + std::forward<_Args>(__args)...) { __glibcxx_assert(holds_alternative<_Tp>(*this)); } template<typename _Tp, typename _Up, typename... _Args, @@ -983,13 +1167,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _Default_ctor_enabler(_Enable_default_constructor_tag{}) { __glibcxx_assert(index() == _Np); } - ~variant() = default; - - variant& operator=(const variant&) = default; - variant& operator=(variant&&) - noexcept((is_nothrow_move_constructible_v<_Types> && ...) - && (is_nothrow_move_assignable_v<_Types> && ...)) = default; - template<typename _Tp> enable_if_t<__exactly_once<__accepted_type<_Tp&&>> && is_constructible_v<__accepted_type<_Tp&&>, _Tp&&> @@ -1084,7 +1261,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION constexpr size_t index() const noexcept { if (this->_M_index == - typename _Base::_Storage::__index_type(variant_npos)) + typename _Base::__index_type(variant_npos)) return variant_npos; return this->_M_index; } diff --git a/libstdc++-v3/testsuite/20_util/variant/compile.cc b/libstdc++-v3/testsuite/20_util/variant/compile.cc index 06e8eb31ee8..dbf5cffac57 100644 --- a/libstdc++-v3/testsuite/20_util/variant/compile.cc +++ b/libstdc++-v3/testsuite/20_util/variant/compile.cc @@ -88,10 +88,12 @@ void copy_ctor() { static_assert(is_copy_constructible_v<variant<int, string>>, ""); static_assert(!is_copy_constructible_v<variant<AllDeleted, string>>, ""); + static_assert(is_trivially_copy_constructible_v<variant<int>>, ""); + static_assert(!is_trivially_copy_constructible_v<variant<std::string>>, ""); { variant<int> a; - static_assert(!noexcept(variant<int>(a)), ""); + static_assert(noexcept(variant<int>(a)), ""); } { variant<string> a; @@ -103,7 +105,7 @@ void copy_ctor() } { variant<int, char> a; - static_assert(!noexcept(variant<int, char>(a)), ""); + static_assert(noexcept(variant<int, char>(a)), ""); } } @@ -111,6 +113,8 @@ void move_ctor() { static_assert(is_move_constructible_v<variant<int, string>>, ""); static_assert(!is_move_constructible_v<variant<AllDeleted, string>>, ""); + static_assert(is_trivially_move_constructible_v<variant<int>>, ""); + static_assert(!is_trivially_move_constructible_v<variant<std::string>>, ""); static_assert(!noexcept(variant<int, Empty>(declval<variant<int, Empty>>())), ""); static_assert(noexcept(variant<int, DefaultNoexcept>(declval<variant<int, DefaultNoexcept>>())), ""); } @@ -148,13 +152,15 @@ void copy_assign() { static_assert(is_copy_assignable_v<variant<int, string>>, ""); static_assert(!is_copy_assignable_v<variant<AllDeleted, string>>, ""); + static_assert(is_trivially_copy_assignable_v<variant<int>>, ""); + static_assert(!is_trivially_copy_assignable_v<variant<string>>, ""); { variant<Empty> a; static_assert(!noexcept(a = a), ""); } { variant<DefaultNoexcept> a; - static_assert(!noexcept(a = a), ""); + static_assert(noexcept(a = a), ""); } } @@ -162,6 +168,8 @@ void move_assign() { static_assert(is_move_assignable_v<variant<int, string>>, ""); static_assert(!is_move_assignable_v<variant<AllDeleted, string>>, ""); + static_assert(is_trivially_move_assignable_v<variant<int>>, ""); + static_assert(!is_trivially_move_assignable_v<variant<string>>, ""); { variant<Empty> a; static_assert(!noexcept(a = std::move(a)), ""); @@ -454,3 +462,75 @@ void test_emplace() static_assert(!has_type_emplace<variant<AllDeleted>, AllDeleted>(0), ""); static_assert(!has_index_emplace<variant<AllDeleted>, 0>(0), ""); } + +void test_triviality() +{ +#define TEST_TEMPLATE(CC, MC, CA, MA, CC_VAL, MC_VAL, CA_VAL, MA_VAL) \ + { \ + struct A \ + { \ + A(const A&) CC; \ + A(A&&) MC; \ + A& operator=(const A&) CA; \ + A& operator=(A&&) MA; \ + }; \ + static_assert(CC_VAL is_trivially_copy_constructible_v<variant<A>>, ""); \ + static_assert(MC_VAL is_trivially_move_constructible_v<variant<A>>, ""); \ + static_assert(CA_VAL is_trivially_copy_assignable_v<variant<A>>, ""); \ + static_assert(MA_VAL is_trivially_move_assignable_v<variant<A>>, ""); \ + } + TEST_TEMPLATE(=default, =default, =default, =default, , , ,) + TEST_TEMPLATE(=default, =default, =default, {}, , , ,!) + TEST_TEMPLATE(=default, =default, {}, =default, , , !,) + TEST_TEMPLATE(=default, =default, {}, {}, , , !,!) + TEST_TEMPLATE(=default, {}, =default, =default, , !, ,) + TEST_TEMPLATE(=default, {}, =default, {}, , !, ,!) + TEST_TEMPLATE(=default, {}, {}, =default, , !, !,) + TEST_TEMPLATE(=default, {}, {}, {}, , !, !,!) + TEST_TEMPLATE({}, =default, =default, =default, !, , ,) + TEST_TEMPLATE({}, =default, =default, {}, !, , ,!) + TEST_TEMPLATE({}, =default, {}, =default, !, , !,) + TEST_TEMPLATE({}, =default, {}, {}, !, , !,!) + TEST_TEMPLATE({}, {}, =default, =default, !, !, ,) + TEST_TEMPLATE({}, {}, =default, {}, !, !, ,!) + TEST_TEMPLATE({}, {}, {}, =default, !, !, !,) + TEST_TEMPLATE({}, {}, {}, {}, !, !, !,!) +#undef TEST_TEMPLATE + +#define TEST_TEMPLATE(CC, MC, CA, MA) \ + { \ + struct A \ + { \ + A(const A&) CC; \ + A(A&&) MC; \ + A& operator=(const A&) CA; \ + A& operator=(A&&) MA; \ + }; \ + static_assert(!is_trivially_copy_constructible_v<variant<AllDeleted, A>>, ""); \ + static_assert(!is_trivially_move_constructible_v<variant<AllDeleted, A>>, ""); \ + static_assert(!is_trivially_copy_assignable_v<variant<AllDeleted, A>>, ""); \ + static_assert(!is_trivially_move_assignable_v<variant<AllDeleted, A>>, ""); \ + } + TEST_TEMPLATE(=default, =default, =default, =default) + TEST_TEMPLATE(=default, =default, =default, {}) + TEST_TEMPLATE(=default, =default, {}, =default) + TEST_TEMPLATE(=default, =default, {}, {}) + TEST_TEMPLATE(=default, {}, =default, =default) + TEST_TEMPLATE(=default, {}, =default, {}) + TEST_TEMPLATE(=default, {}, {}, =default) + TEST_TEMPLATE(=default, {}, {}, {}) + TEST_TEMPLATE({}, =default, =default, =default) + TEST_TEMPLATE({}, =default, =default, {}) + TEST_TEMPLATE({}, =default, {}, =default) + TEST_TEMPLATE({}, =default, {}, {}) + TEST_TEMPLATE({}, {}, =default, =default) + TEST_TEMPLATE({}, {}, =default, {}) + TEST_TEMPLATE({}, {}, {}, =default) + TEST_TEMPLATE({}, {}, {}, {}) +#undef TEST_TEMPLATE + + static_assert(is_trivially_copy_constructible_v<variant<DefaultNoexcept, int, char, float, double>>, ""); + static_assert(is_trivially_move_constructible_v<variant<DefaultNoexcept, int, char, float, double>>, ""); + static_assert(is_trivially_copy_assignable_v<variant<DefaultNoexcept, int, char, float, double>>, ""); + static_assert(is_trivially_move_assignable_v<variant<DefaultNoexcept, int, char, float, double>>, ""); +} ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch] Forward triviality in variant 2017-05-30 6:36 [Patch] Forward triviality in variant Tim Shen via gcc-patches @ 2017-05-30 9:41 ` Tim Shen via gcc-patches 2017-06-01 15:13 ` Jonathan Wakely 0 siblings, 1 reply; 11+ messages in thread From: Tim Shen via gcc-patches @ 2017-05-30 9:41 UTC (permalink / raw) To: libstdc++, gcc-patches [-- Attachment #1: Type: text/plain, Size: 289 bytes --] On Mon, May 29, 2017 at 11:29 PM, Tim Shen <timshen@google.com> wrote: > This patch implements > <https://lichray.github.io/trivially_variant.html>, but with more Actually, it didn't. The copy assign and move assign conditions are wrong in the patch. Fixed those. -- Regards, Tim Shen [-- Attachment #2: b.diff --] [-- Type: text/x-patch, Size: 26360 bytes --] commit 03387ef5007e171e4aeceeddf4d856caffeedb41 Author: Tim Shen <timshen@google.com> Date: Mon May 29 22:44:42 2017 -0700 2017-05-30 Tim Shen <timshen@google.com> PR libstdc++/80187 * include/std/variant (variant::variant, variant::~variant, variant::operator=): Implement triviality forwarding for four special member functions. * testsuite/20_util/variant/compile.cc: Tests. diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant index b9824a5182c..f81b815af09 100644 --- a/libstdc++-v3/include/std/variant +++ b/libstdc++-v3/include/std/variant @@ -290,6 +290,53 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __ref_cast<_Tp>(__t)); } + template<typename... _Types> + struct _Traits + { + static constexpr bool is_default_constructible_v = + is_default_constructible_v<typename _Nth_type<0, _Types...>::type>; + static constexpr bool is_copy_constructible_v = + __and_<is_copy_constructible<_Types>...>::value; + static constexpr bool is_move_constructible_v = + __and_<is_move_constructible<_Types>...>::value; + static constexpr bool is_copy_assignable_v = + is_copy_constructible_v && is_move_constructible_v + && __and_<is_copy_assignable<_Types>...>::value; + static constexpr bool is_move_assignable_v = + is_move_constructible_v + && __and_<is_move_assignable<_Types>...>::value; + + static constexpr bool is_dtor_trivial = + __and_<is_trivially_destructible<_Types>...>::value; + static constexpr bool is_copy_ctor_trivial = + __and_<is_trivially_copy_constructible<_Types>...>::value; + static constexpr bool is_move_ctor_trivial = + __and_<is_trivially_move_constructible<_Types>...>::value; + static constexpr bool is_copy_assign_trivial = + is_dtor_trivial + && is_copy_ctor_trivial + && __and_<is_trivially_copy_assignable<_Types>...>::value; + static constexpr bool is_move_assign_trivial = + is_dtor_trivial + && is_move_ctor_trivial + && __and_<is_trivially_move_assignable<_Types>...>::value; + + static constexpr bool is_default_ctor_noexcept = + is_nothrow_default_constructible_v< + typename _Nth_type<0, _Types...>::type>; + static constexpr bool is_copy_ctor_noexcept = + is_copy_ctor_trivial; + static constexpr bool is_move_ctor_noexcept = + is_move_ctor_trivial + || __and_<is_nothrow_move_constructible<_Types>...>::value; + static constexpr bool is_copy_assign_noexcept = + is_copy_assign_trivial; + static constexpr bool is_move_assign_noexcept = + is_move_assign_trivial || + (is_move_ctor_noexcept + && __and_<is_nothrow_move_assignable<_Types>...>::value); + }; + // Defines members and ctors. template<typename... _Types> union _Variadic_union { }; @@ -355,6 +402,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION ~_Variant_storage() { _M_reset(); } + void* + _M_storage() const + { + return const_cast<void*>(static_cast<const void*>( + std::addressof(_M_u))); + } + + constexpr bool + _M_valid() const noexcept + { + return this->_M_index != __index_type(variant_npos); + } + _Variadic_union<_Types...> _M_u; using __index_type = __select_index<_Types...>; __index_type _M_index; @@ -374,59 +434,114 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION void _M_reset() { _M_index = variant_npos; } + void* + _M_storage() const + { + return const_cast<void*>(static_cast<const void*>( + std::addressof(_M_u))); + } + + constexpr bool + _M_valid() const noexcept + { + return this->_M_index != __index_type(variant_npos); + } + _Variadic_union<_Types...> _M_u; using __index_type = __select_index<_Types...>; __index_type _M_index; }; - // Helps SFINAE on special member functions. Otherwise it can live in variant - // class. template<typename... _Types> - struct _Variant_base : - _Variant_storage<(std::is_trivially_destructible_v<_Types> && ...), - _Types...> - { - using _Storage = - _Variant_storage<(std::is_trivially_destructible_v<_Types> && ...), - _Types...>; + using _Variant_storage_alias = + _Variant_storage<_Traits<_Types...>::is_dtor_trivial, _Types...>; - constexpr - _Variant_base() - noexcept(is_nothrow_default_constructible_v< - variant_alternative_t<0, variant<_Types...>>>) - : _Variant_base(in_place_index<0>) { } + // The following are (Copy|Move) (ctor|assign) layers for forwarding + // triviality and handling non-trivial SMF behaviors. - _Variant_base(const _Variant_base& __rhs) + template<bool, typename... _Types> + struct _Copy_ctor_base : _Variant_storage_alias<_Types...> + { + using _Base = _Variant_storage_alias<_Types...>; + using _Base::_Base; + + _Copy_ctor_base(const _Copy_ctor_base& __rhs) + noexcept(_Traits<_Types...>::is_copy_ctor_noexcept) { if (__rhs._M_valid()) { static constexpr void (*_S_vtable[])(void*, void*) = { &__erased_ctor<_Types&, const _Types&>... }; - _S_vtable[__rhs._M_index](_M_storage(), __rhs._M_storage()); + _S_vtable[__rhs._M_index](this->_M_storage(), __rhs._M_storage()); this->_M_index = __rhs._M_index; } } - _Variant_base(_Variant_base&& __rhs) - noexcept((is_nothrow_move_constructible_v<_Types> && ...)) + _Copy_ctor_base(_Copy_ctor_base&&) + noexcept(_Traits<_Types...>::is_move_ctor_noexcept) = default; + _Copy_ctor_base& operator=(const _Copy_ctor_base&) + noexcept(_Traits<_Types...>::is_copy_assign_noexcept) = default; + _Copy_ctor_base& operator=(_Copy_ctor_base&&) + noexcept(_Traits<_Types...>::is_move_assign_noexcept) = default; + }; + + template<typename... _Types> + struct _Copy_ctor_base<true, _Types...> : _Variant_storage_alias<_Types...> + { + using _Base = _Variant_storage_alias<_Types...>; + using _Base::_Base; + }; + + template<typename... _Types> + using _Copy_ctor_alias = + _Copy_ctor_base<_Traits<_Types...>::is_copy_ctor_trivial, _Types...>; + + template<bool, typename... _Types> + struct _Move_ctor_base : _Copy_ctor_alias<_Types...> + { + using _Base = _Copy_ctor_alias<_Types...>; + using _Base::_Base; + + _Move_ctor_base(_Move_ctor_base&& __rhs) + noexcept(_Traits<_Types...>::is_move_ctor_noexcept) { if (__rhs._M_valid()) { static constexpr void (*_S_vtable[])(void*, void*) = { &__erased_ctor<_Types&, _Types&&>... }; - _S_vtable[__rhs._M_index](_M_storage(), __rhs._M_storage()); + _S_vtable[__rhs._M_index](this->_M_storage(), __rhs._M_storage()); this->_M_index = __rhs._M_index; } } - template<size_t _Np, typename... _Args> - constexpr explicit - _Variant_base(in_place_index_t<_Np> __i, _Args&&... __args) - : _Storage(__i, std::forward<_Args>(__args)...) - { } + _Move_ctor_base(const _Move_ctor_base& __rhs) + noexcept(_Traits<_Types...>::is_copy_ctor_noexcept) = default; + _Move_ctor_base& operator=(const _Move_ctor_base&) + noexcept(_Traits<_Types...>::is_copy_assign_noexcept) = default; + _Move_ctor_base& operator=(_Move_ctor_base&&) + noexcept(_Traits<_Types...>::is_move_assign_noexcept) = default; + }; + + template<typename... _Types> + struct _Move_ctor_base<true, _Types...> : _Copy_ctor_alias<_Types...> + { + using _Base = _Copy_ctor_alias<_Types...>; + using _Base::_Base; + }; + + template<typename... _Types> + using _Move_ctor_alias = + _Move_ctor_base<_Traits<_Types...>::is_move_ctor_trivial, _Types...>; + + template<bool, typename... _Types> + struct _Copy_assign_base : _Move_ctor_alias<_Types...> + { + using _Base = _Move_ctor_alias<_Types...>; + using _Base::_Base; - _Variant_base& - operator=(const _Variant_base& __rhs) + _Copy_assign_base& + operator=(const _Copy_assign_base& __rhs) + noexcept(_Traits<_Types...>::is_copy_assign_noexcept) { if (this->_M_index == __rhs._M_index) { @@ -434,16 +549,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { static constexpr void (*_S_vtable[])(void*, void*) = { &__erased_assign<_Types&, const _Types&>... }; - _S_vtable[__rhs._M_index](_M_storage(), __rhs._M_storage()); + _S_vtable[__rhs._M_index](this->_M_storage(), __rhs._M_storage()); } } else { - _Variant_base __tmp(__rhs); - this->~_Variant_base(); + _Copy_assign_base __tmp(__rhs); + this->~_Copy_assign_base(); __try { - ::new (this) _Variant_base(std::move(__tmp)); + ::new (this) _Copy_assign_base(std::move(__tmp)); } __catch (...) { @@ -455,12 +570,38 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION return *this; } - void _M_destructive_move(_Variant_base&& __rhs) + _Copy_assign_base(const _Copy_assign_base& __rhs) + noexcept(_Traits<_Types...>::is_copy_ctor_noexcept) = default; + _Copy_assign_base(_Copy_assign_base&&) + noexcept(_Traits<_Types...>::is_move_ctor_noexcept) = default; + _Copy_assign_base& operator=(_Copy_assign_base&&) + noexcept(_Traits<_Types...>::is_move_assign_noexcept) = default; + }; + + template<typename... _Types> + struct _Copy_assign_base<true, _Types...> : _Move_ctor_alias<_Types...> + { + using _Base = _Move_ctor_alias<_Types...>; + using _Base::_Base; + }; + + template<typename... _Types> + using _Copy_assign_alias = + _Copy_assign_base<_Traits<_Types...>::is_copy_assign_trivial, + _Types...>; + + template<bool, typename... _Types> + struct _Move_assign_base : _Copy_assign_alias<_Types...> + { + using _Base = _Copy_assign_alias<_Types...>; + using _Base::_Base; + + void _M_destructive_move(_Move_assign_base&& __rhs) { - this->~_Variant_base(); + this->~_Move_assign_base(); __try { - ::new (this) _Variant_base(std::move(__rhs)); + ::new (this) _Move_assign_base(std::move(__rhs)); } __catch (...) { @@ -469,40 +610,81 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } } - _Variant_base& - operator=(_Variant_base&& __rhs) - noexcept((is_nothrow_move_constructible_v<_Types> && ...) - && (is_nothrow_move_assignable_v<_Types> && ...)) + _Move_assign_base& + operator=(_Move_assign_base&& __rhs) + noexcept(_Traits<_Types...>::is_move_assign_noexcept) { if (this->_M_index == __rhs._M_index) { if (__rhs._M_valid()) { static constexpr void (*_S_vtable[])(void*, void*) = - { &__erased_assign<_Types&, _Types&&>... }; - _S_vtable[__rhs._M_index](_M_storage(), __rhs._M_storage()); + { &__erased_assign<_Types&, const _Types&>... }; + _S_vtable[__rhs._M_index](this->_M_storage(), __rhs._M_storage()); } } else { - _M_destructive_move(std::move(__rhs)); + _Move_assign_base __tmp(__rhs); + this->~_Move_assign_base(); + __try + { + ::new (this) _Move_assign_base(std::move(__tmp)); + } + __catch (...) + { + this->_M_index = variant_npos; + __throw_exception_again; + } } + __glibcxx_assert(this->_M_index == __rhs._M_index); return *this; } - void* - _M_storage() const - { - return const_cast<void*>(static_cast<const void*>( - std::addressof(_Storage::_M_u))); - } + _Move_assign_base(const _Move_assign_base& __rhs) + noexcept(_Traits<_Types...>::is_copy_ctor_noexcept) = default; + _Move_assign_base(_Move_assign_base&&) + noexcept(_Traits<_Types...>::is_move_ctor_noexcept) = default; + _Move_assign_base& operator=(const _Move_assign_base&) + noexcept(_Traits<_Types...>::is_copy_assign_noexcept) = default; + }; - constexpr bool - _M_valid() const noexcept - { - return this->_M_index != - typename _Storage::__index_type(variant_npos); - } + template<typename... _Types> + struct _Move_assign_base<true, _Types...> : _Copy_assign_alias<_Types...> + { + using _Base = _Copy_assign_alias<_Types...>; + using _Base::_Base; + }; + + template<typename... _Types> + using _Move_assign_alias = + _Move_assign_base<_Traits<_Types...>::is_move_assign_trivial, + _Types...>; + + template<typename... _Types> + struct _Variant_base : _Move_assign_alias<_Types...> + { + using _Base = _Move_assign_alias<_Types...>; + + constexpr + _Variant_base() + noexcept(_Traits<_Types...>::is_default_ctor_noexcept) + : _Variant_base(in_place_index<0>) { } + + template<size_t _Np, typename... _Args> + constexpr explicit + _Variant_base(in_place_index_t<_Np> __i, _Args&&... __args) + : _Base(__i, std::forward<_Args>(__args)...) + { } + + _Variant_base(const _Variant_base& __rhs) + noexcept(_Traits<_Types...>::is_copy_ctor_noexcept) = default; + _Variant_base(_Variant_base&&) + noexcept(_Traits<_Types...>::is_move_ctor_noexcept) = default; + _Variant_base& operator=(const _Variant_base&) + noexcept(_Traits<_Types...>::is_copy_assign_noexcept) = default; + _Variant_base& operator=(_Variant_base&&) + noexcept(_Traits<_Types...>::is_move_assign_noexcept) = default; }; // For how many times does _Tp appear in _Tuple? @@ -877,16 +1059,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION class variant : private __detail::__variant::_Variant_base<_Types...>, private _Enable_default_constructor< - is_default_constructible_v< - variant_alternative_t<0, variant<_Types...>>>, variant<_Types...>>, + __detail::__variant::_Traits<_Types...>::is_default_constructible_v, + variant<_Types...>>, private _Enable_copy_move< - (is_copy_constructible_v<_Types> && ...), - (is_copy_constructible_v<_Types> && ...) - && (is_move_constructible_v<_Types> && ...) - && (is_copy_assignable_v<_Types> && ...), - (is_move_constructible_v<_Types> && ...), - (is_move_constructible_v<_Types> && ...) - && (is_move_assignable_v<_Types> && ...), + __detail::__variant::_Traits<_Types...>::is_copy_constructible_v, + __detail::__variant::_Traits<_Types...>::is_copy_assignable_v, + __detail::__variant::_Traits<_Types...>::is_move_constructible_v, + __detail::__variant::_Traits<_Types...>::is_move_assignable_v, variant<_Types...>> { private: @@ -899,9 +1078,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION using _Base = __detail::__variant::_Variant_base<_Types...>; using _Default_ctor_enabler = - _Enable_default_constructor< - is_default_constructible_v< - variant_alternative_t<0, variant<_Types...>>>, variant<_Types...>>; + _Enable_default_constructor< + __detail::__variant::_Traits<_Types...>::is_default_constructible_v, + variant<_Types...>>; template<typename _Tp> static constexpr bool @@ -928,12 +1107,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION static constexpr size_t __index_of = __detail::__variant::__index_of_v<_Tp, _Types...>; + using _Traits = __detail::__variant::_Traits<_Types...>; + public: - constexpr variant() - noexcept(is_nothrow_default_constructible_v<__to_type<0>>) = default; - variant(const variant&) = default; + variant() noexcept(_Traits::is_default_ctor_noexcept) = default; + variant(const variant& __rhs) + noexcept(_Traits::is_copy_ctor_noexcept) = default; variant(variant&&) - noexcept((is_nothrow_move_constructible_v<_Types> && ...)) = default; + noexcept(_Traits::is_move_ctor_noexcept) = default; + variant& operator=(const variant&) + noexcept(_Traits::is_copy_assign_noexcept) = default; + variant& operator=(variant&&) + noexcept(_Traits::is_move_assign_noexcept) = default; + ~variant() = default; template<typename _Tp, typename = enable_if_t<!is_same_v<decay_t<_Tp>, variant>>, @@ -942,7 +1128,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION constexpr variant(_Tp&& __t) noexcept(is_nothrow_constructible_v<__accepted_type<_Tp&&>, _Tp&&>) - : variant(in_place_index<__accepted_index<_Tp&&>>, std::forward<_Tp>(__t)) + : variant(in_place_index<__accepted_index<_Tp&&>>, + std::forward<_Tp>(__t)) { __glibcxx_assert(holds_alternative<__accepted_type<_Tp&&>>(*this)); } template<typename _Tp, typename... _Args, @@ -950,7 +1137,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION && is_constructible_v<_Tp, _Args&&...>>> constexpr explicit variant(in_place_type_t<_Tp>, _Args&&... __args) - : variant(in_place_index<__index_of<_Tp>>, std::forward<_Args>(__args)...) + : variant(in_place_index<__index_of<_Tp>>, + std::forward<_Args>(__args)...) { __glibcxx_assert(holds_alternative<_Tp>(*this)); } template<typename _Tp, typename _Up, typename... _Args, @@ -983,13 +1171,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _Default_ctor_enabler(_Enable_default_constructor_tag{}) { __glibcxx_assert(index() == _Np); } - ~variant() = default; - - variant& operator=(const variant&) = default; - variant& operator=(variant&&) - noexcept((is_nothrow_move_constructible_v<_Types> && ...) - && (is_nothrow_move_assignable_v<_Types> && ...)) = default; - template<typename _Tp> enable_if_t<__exactly_once<__accepted_type<_Tp&&>> && is_constructible_v<__accepted_type<_Tp&&>, _Tp&&> @@ -1084,7 +1265,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION constexpr size_t index() const noexcept { if (this->_M_index == - typename _Base::_Storage::__index_type(variant_npos)) + typename _Base::__index_type(variant_npos)) return variant_npos; return this->_M_index; } diff --git a/libstdc++-v3/testsuite/20_util/variant/compile.cc b/libstdc++-v3/testsuite/20_util/variant/compile.cc index 06e8eb31ee8..2e8fc341d4d 100644 --- a/libstdc++-v3/testsuite/20_util/variant/compile.cc +++ b/libstdc++-v3/testsuite/20_util/variant/compile.cc @@ -88,10 +88,12 @@ void copy_ctor() { static_assert(is_copy_constructible_v<variant<int, string>>, ""); static_assert(!is_copy_constructible_v<variant<AllDeleted, string>>, ""); + static_assert(is_trivially_copy_constructible_v<variant<int>>, ""); + static_assert(!is_trivially_copy_constructible_v<variant<std::string>>, ""); { variant<int> a; - static_assert(!noexcept(variant<int>(a)), ""); + static_assert(noexcept(variant<int>(a)), ""); } { variant<string> a; @@ -103,7 +105,7 @@ void copy_ctor() } { variant<int, char> a; - static_assert(!noexcept(variant<int, char>(a)), ""); + static_assert(noexcept(variant<int, char>(a)), ""); } } @@ -111,6 +113,8 @@ void move_ctor() { static_assert(is_move_constructible_v<variant<int, string>>, ""); static_assert(!is_move_constructible_v<variant<AllDeleted, string>>, ""); + static_assert(is_trivially_move_constructible_v<variant<int>>, ""); + static_assert(!is_trivially_move_constructible_v<variant<std::string>>, ""); static_assert(!noexcept(variant<int, Empty>(declval<variant<int, Empty>>())), ""); static_assert(noexcept(variant<int, DefaultNoexcept>(declval<variant<int, DefaultNoexcept>>())), ""); } @@ -148,13 +152,15 @@ void copy_assign() { static_assert(is_copy_assignable_v<variant<int, string>>, ""); static_assert(!is_copy_assignable_v<variant<AllDeleted, string>>, ""); + static_assert(is_trivially_copy_assignable_v<variant<int>>, ""); + static_assert(!is_trivially_copy_assignable_v<variant<string>>, ""); { variant<Empty> a; static_assert(!noexcept(a = a), ""); } { variant<DefaultNoexcept> a; - static_assert(!noexcept(a = a), ""); + static_assert(noexcept(a = a), ""); } } @@ -162,6 +168,8 @@ void move_assign() { static_assert(is_move_assignable_v<variant<int, string>>, ""); static_assert(!is_move_assignable_v<variant<AllDeleted, string>>, ""); + static_assert(is_trivially_move_assignable_v<variant<int>>, ""); + static_assert(!is_trivially_move_assignable_v<variant<string>>, ""); { variant<Empty> a; static_assert(!noexcept(a = std::move(a)), ""); @@ -454,3 +462,92 @@ void test_emplace() static_assert(!has_type_emplace<variant<AllDeleted>, AllDeleted>(0), ""); static_assert(!has_index_emplace<variant<AllDeleted>, 0>(0), ""); } + +void test_triviality() +{ +#define TEST_TEMPLATE(DT, CC, MC, CA, MA, CC_VAL, MC_VAL, CA_VAL, MA_VAL) \ + { \ + struct A \ + { \ + ~A() DT; \ + A(const A&) CC; \ + A(A&&) MC; \ + A& operator=(const A&) CA; \ + A& operator=(A&&) MA; \ + }; \ + static_assert(CC_VAL == is_trivially_copy_constructible_v<variant<A>>, ""); \ + static_assert(MC_VAL == is_trivially_move_constructible_v<variant<A>>, ""); \ + static_assert(CA_VAL == is_trivially_copy_assignable_v<variant<A>>, ""); \ + static_assert(MA_VAL == is_trivially_move_assignable_v<variant<A>>, ""); \ + } + TEST_TEMPLATE(=default, =default, =default, =default, =default, true, true, true, true) + TEST_TEMPLATE(=default, =default, =default, =default, {}, true, true, true, false) + TEST_TEMPLATE(=default, =default, =default, {}, =default, true, true, false, true) + TEST_TEMPLATE(=default, =default, =default, {}, {}, true, true, false, false) + TEST_TEMPLATE(=default, =default, {}, =default, =default, true, false, true, false) + TEST_TEMPLATE(=default, =default, {}, =default, {}, true, false, true, false) + TEST_TEMPLATE(=default, =default, {}, {}, =default, true, false, false, false) + TEST_TEMPLATE(=default, =default, {}, {}, {}, true, false, false, false) + TEST_TEMPLATE(=default, {}, =default, =default, =default, false, true, false, true) + TEST_TEMPLATE(=default, {}, =default, =default, {}, false, true, false, false) + TEST_TEMPLATE(=default, {}, =default, {}, =default, false, true, false, true) + TEST_TEMPLATE(=default, {}, =default, {}, {}, false, true, false, false) + TEST_TEMPLATE(=default, {}, {}, =default, =default, false, false, false, false) + TEST_TEMPLATE(=default, {}, {}, =default, {}, false, false, false, false) + TEST_TEMPLATE(=default, {}, {}, {}, =default, false, false, false, false) + TEST_TEMPLATE(=default, {}, {}, {}, {}, false, false, false, false) + TEST_TEMPLATE( {}, =default, =default, =default, =default, false, false, false, false) + TEST_TEMPLATE( {}, =default, =default, =default, {}, false, false, false, false) + TEST_TEMPLATE( {}, =default, =default, {}, =default, false, false, false, false) + TEST_TEMPLATE( {}, =default, =default, {}, {}, false, false, false, false) + TEST_TEMPLATE( {}, =default, {}, =default, =default, false, false, false, false) + TEST_TEMPLATE( {}, =default, {}, =default, {}, false, false, false, false) + TEST_TEMPLATE( {}, =default, {}, {}, =default, false, false, false, false) + TEST_TEMPLATE( {}, =default, {}, {}, {}, false, false, false, false) + TEST_TEMPLATE( {}, {}, =default, =default, =default, false, false, false, false) + TEST_TEMPLATE( {}, {}, =default, =default, {}, false, false, false, false) + TEST_TEMPLATE( {}, {}, =default, {}, =default, false, false, false, false) + TEST_TEMPLATE( {}, {}, =default, {}, {}, false, false, false, false) + TEST_TEMPLATE( {}, {}, {}, =default, =default, false, false, false, false) + TEST_TEMPLATE( {}, {}, {}, =default, {}, false, false, false, false) + TEST_TEMPLATE( {}, {}, {}, {}, =default, false, false, false, false) + TEST_TEMPLATE( {}, {}, {}, {}, {}, false, false, false, false) +#undef TEST_TEMPLATE + +#define TEST_TEMPLATE(CC, MC, CA, MA) \ + { \ + struct A \ + { \ + A(const A&) CC; \ + A(A&&) MC; \ + A& operator=(const A&) CA; \ + A& operator=(A&&) MA; \ + }; \ + static_assert(!is_trivially_copy_constructible_v<variant<AllDeleted, A>>, ""); \ + static_assert(!is_trivially_move_constructible_v<variant<AllDeleted, A>>, ""); \ + static_assert(!is_trivially_copy_assignable_v<variant<AllDeleted, A>>, ""); \ + static_assert(!is_trivially_move_assignable_v<variant<AllDeleted, A>>, ""); \ + } + TEST_TEMPLATE(=default, =default, =default, =default) + TEST_TEMPLATE(=default, =default, =default, {}) + TEST_TEMPLATE(=default, =default, {}, =default) + TEST_TEMPLATE(=default, =default, {}, {}) + TEST_TEMPLATE(=default, {}, =default, =default) + TEST_TEMPLATE(=default, {}, =default, {}) + TEST_TEMPLATE(=default, {}, {}, =default) + TEST_TEMPLATE(=default, {}, {}, {}) + TEST_TEMPLATE( {}, =default, =default, =default) + TEST_TEMPLATE( {}, =default, =default, {}) + TEST_TEMPLATE( {}, =default, {}, =default) + TEST_TEMPLATE( {}, =default, {}, {}) + TEST_TEMPLATE( {}, {}, =default, =default) + TEST_TEMPLATE( {}, {}, =default, {}) + TEST_TEMPLATE( {}, {}, {}, =default) + TEST_TEMPLATE( {}, {}, {}, {}) +#undef TEST_TEMPLATE + + static_assert(is_trivially_copy_constructible_v<variant<DefaultNoexcept, int, char, float, double>>, ""); + static_assert(is_trivially_move_constructible_v<variant<DefaultNoexcept, int, char, float, double>>, ""); + static_assert(is_trivially_copy_assignable_v<variant<DefaultNoexcept, int, char, float, double>>, ""); + static_assert(is_trivially_move_assignable_v<variant<DefaultNoexcept, int, char, float, double>>, ""); +} ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch] Forward triviality in variant 2017-05-30 9:41 ` Tim Shen via gcc-patches @ 2017-06-01 15:13 ` Jonathan Wakely 2017-06-01 15:21 ` Ville Voutilainen 2017-06-18 19:37 ` Tim Shen via gcc-patches 0 siblings, 2 replies; 11+ messages in thread From: Jonathan Wakely @ 2017-06-01 15:13 UTC (permalink / raw) To: Tim Shen; +Cc: libstdc++, gcc-patches On 30/05/17 02:16 -0700, Tim Shen via libstdc++ wrote: >diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant >index b9824a5182c..f81b815af09 100644 >--- a/libstdc++-v3/include/std/variant >+++ b/libstdc++-v3/include/std/variant >@@ -290,6 +290,53 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > __ref_cast<_Tp>(__t)); > } > >+ template<typename... _Types> >+ struct _Traits >+ { >+ static constexpr bool is_default_constructible_v = >+ is_default_constructible_v<typename _Nth_type<0, _Types...>::type>; >+ static constexpr bool is_copy_constructible_v = >+ __and_<is_copy_constructible<_Types>...>::value; >+ static constexpr bool is_move_constructible_v = >+ __and_<is_move_constructible<_Types>...>::value; >+ static constexpr bool is_copy_assignable_v = >+ is_copy_constructible_v && is_move_constructible_v >+ && __and_<is_copy_assignable<_Types>...>::value; >+ static constexpr bool is_move_assignable_v = >+ is_move_constructible_v >+ && __and_<is_move_assignable<_Types>...>::value; It seems strange to me that these ones end with _v but the following ones don't. Could we make them all have no _v suffix? >+ static constexpr bool is_dtor_trivial = >+ __and_<is_trivially_destructible<_Types>...>::value; >+ static constexpr bool is_copy_ctor_trivial = >+ __and_<is_trivially_copy_constructible<_Types>...>::value; >+ static constexpr bool is_move_ctor_trivial = >+ __and_<is_trivially_move_constructible<_Types>...>::value; >+ static constexpr bool is_copy_assign_trivial = >+ is_dtor_trivial >+ && is_copy_ctor_trivial >+ && __and_<is_trivially_copy_assignable<_Types>...>::value; >+ static constexpr bool is_move_assign_trivial = >+ is_dtor_trivial >+ && is_move_ctor_trivial >+ && __and_<is_trivially_move_assignable<_Types>...>::value; >+ >+ static constexpr bool is_default_ctor_noexcept = >+ is_nothrow_default_constructible_v< >+ typename _Nth_type<0, _Types...>::type>; >+ static constexpr bool is_copy_ctor_noexcept = >+ is_copy_ctor_trivial; >+ static constexpr bool is_move_ctor_noexcept = >+ is_move_ctor_trivial >+ || __and_<is_nothrow_move_constructible<_Types>...>::value; >+ static constexpr bool is_copy_assign_noexcept = >+ is_copy_assign_trivial; >+ static constexpr bool is_move_assign_noexcept = >+ is_move_assign_trivial || >+ (is_move_ctor_noexcept >+ && __and_<is_nothrow_move_assignable<_Types>...>::value); >+ }; Does using __and_ for any of those traits reduce the limit on the number of alternatives in a variant? We switched to using fold expressions in some contexts to avoid very deep instantiations, but I don't know if these will hit the same problem, but it looks like it will. > // Defines members and ctors. > template<typename... _Types> > union _Variadic_union { }; >@@ -355,6 +402,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > ~_Variant_storage() > { _M_reset(); } > >+ void* >+ _M_storage() const >+ { >+ return const_cast<void*>(static_cast<const void*>( >+ std::addressof(_M_u))); >+ } >+ >+ constexpr bool >+ _M_valid() const noexcept >+ { >+ return this->_M_index != __index_type(variant_npos); >+ } >+ > _Variadic_union<_Types...> _M_u; > using __index_type = __select_index<_Types...>; > __index_type _M_index; >@@ -374,59 +434,114 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > void _M_reset() > { _M_index = variant_npos; } > >+ void* >+ _M_storage() const >+ { >+ return const_cast<void*>(static_cast<const void*>( >+ std::addressof(_M_u))); >+ } >+ >+ constexpr bool >+ _M_valid() const noexcept >+ { >+ return this->_M_index != __index_type(variant_npos); >+ } >+ > _Variadic_union<_Types...> _M_u; > using __index_type = __select_index<_Types...>; > __index_type _M_index; > }; > >- // Helps SFINAE on special member functions. Otherwise it can live in variant >- // class. > template<typename... _Types> >- struct _Variant_base : >- _Variant_storage<(std::is_trivially_destructible_v<_Types> && ...), >- _Types...> >- { >- using _Storage = >- _Variant_storage<(std::is_trivially_destructible_v<_Types> && ...), >- _Types...>; >+ using _Variant_storage_alias = >+ _Variant_storage<_Traits<_Types...>::is_dtor_trivial, _Types...>; > >- constexpr >- _Variant_base() >- noexcept(is_nothrow_default_constructible_v< >- variant_alternative_t<0, variant<_Types...>>>) >- : _Variant_base(in_place_index<0>) { } >+ // The following are (Copy|Move) (ctor|assign) layers for forwarding >+ // triviality and handling non-trivial SMF behaviors. > >- _Variant_base(const _Variant_base& __rhs) >+ template<bool, typename... _Types> >+ struct _Copy_ctor_base : _Variant_storage_alias<_Types...> >+ { >+ using _Base = _Variant_storage_alias<_Types...>; >+ using _Base::_Base; >+ >+ _Copy_ctor_base(const _Copy_ctor_base& __rhs) >+ noexcept(_Traits<_Types...>::is_copy_ctor_noexcept) > { > if (__rhs._M_valid()) > { > static constexpr void (*_S_vtable[])(void*, void*) = > { &__erased_ctor<_Types&, const _Types&>... }; >- _S_vtable[__rhs._M_index](_M_storage(), __rhs._M_storage()); >+ _S_vtable[__rhs._M_index](this->_M_storage(), __rhs._M_storage()); > this->_M_index = __rhs._M_index; > } > } > >- _Variant_base(_Variant_base&& __rhs) >- noexcept((is_nothrow_move_constructible_v<_Types> && ...)) >+ _Copy_ctor_base(_Copy_ctor_base&&) >+ noexcept(_Traits<_Types...>::is_move_ctor_noexcept) = default; >+ _Copy_ctor_base& operator=(const _Copy_ctor_base&) >+ noexcept(_Traits<_Types...>::is_copy_assign_noexcept) = default; >+ _Copy_ctor_base& operator=(_Copy_ctor_base&&) >+ noexcept(_Traits<_Types...>::is_move_assign_noexcept) = default; >+ }; >+ >+ template<typename... _Types> >+ struct _Copy_ctor_base<true, _Types...> : _Variant_storage_alias<_Types...> >+ { >+ using _Base = _Variant_storage_alias<_Types...>; >+ using _Base::_Base; >+ }; >+ >+ template<typename... _Types> >+ using _Copy_ctor_alias = >+ _Copy_ctor_base<_Traits<_Types...>::is_copy_ctor_trivial, _Types...>; >+ >+ template<bool, typename... _Types> >+ struct _Move_ctor_base : _Copy_ctor_alias<_Types...> >+ { >+ using _Base = _Copy_ctor_alias<_Types...>; >+ using _Base::_Base; >+ >+ _Move_ctor_base(_Move_ctor_base&& __rhs) >+ noexcept(_Traits<_Types...>::is_move_ctor_noexcept) > { > if (__rhs._M_valid()) > { > static constexpr void (*_S_vtable[])(void*, void*) = > { &__erased_ctor<_Types&, _Types&&>... }; >- _S_vtable[__rhs._M_index](_M_storage(), __rhs._M_storage()); >+ _S_vtable[__rhs._M_index](this->_M_storage(), __rhs._M_storage()); > this->_M_index = __rhs._M_index; > } > } > >- template<size_t _Np, typename... _Args> >- constexpr explicit >- _Variant_base(in_place_index_t<_Np> __i, _Args&&... __args) >- : _Storage(__i, std::forward<_Args>(__args)...) >- { } >+ _Move_ctor_base(const _Move_ctor_base& __rhs) >+ noexcept(_Traits<_Types...>::is_copy_ctor_noexcept) = default; >+ _Move_ctor_base& operator=(const _Move_ctor_base&) >+ noexcept(_Traits<_Types...>::is_copy_assign_noexcept) = default; >+ _Move_ctor_base& operator=(_Move_ctor_base&&) >+ noexcept(_Traits<_Types...>::is_move_assign_noexcept) = default; >+ }; >+ >+ template<typename... _Types> >+ struct _Move_ctor_base<true, _Types...> : _Copy_ctor_alias<_Types...> >+ { >+ using _Base = _Copy_ctor_alias<_Types...>; >+ using _Base::_Base; >+ }; >+ >+ template<typename... _Types> >+ using _Move_ctor_alias = >+ _Move_ctor_base<_Traits<_Types...>::is_move_ctor_trivial, _Types...>; >+ >+ template<bool, typename... _Types> >+ struct _Copy_assign_base : _Move_ctor_alias<_Types...> >+ { >+ using _Base = _Move_ctor_alias<_Types...>; >+ using _Base::_Base; > >- _Variant_base& >- operator=(const _Variant_base& __rhs) >+ _Copy_assign_base& >+ operator=(const _Copy_assign_base& __rhs) >+ noexcept(_Traits<_Types...>::is_copy_assign_noexcept) > { > if (this->_M_index == __rhs._M_index) > { >@@ -434,16 +549,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > { > static constexpr void (*_S_vtable[])(void*, void*) = > { &__erased_assign<_Types&, const _Types&>... }; >- _S_vtable[__rhs._M_index](_M_storage(), __rhs._M_storage()); >+ _S_vtable[__rhs._M_index](this->_M_storage(), __rhs._M_storage()); > } > } > else > { >- _Variant_base __tmp(__rhs); >- this->~_Variant_base(); >+ _Copy_assign_base __tmp(__rhs); >+ this->~_Copy_assign_base(); > __try > { >- ::new (this) _Variant_base(std::move(__tmp)); >+ ::new (this) _Copy_assign_base(std::move(__tmp)); > } > __catch (...) > { >@@ -455,12 +570,38 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > return *this; > } > >- void _M_destructive_move(_Variant_base&& __rhs) >+ _Copy_assign_base(const _Copy_assign_base& __rhs) >+ noexcept(_Traits<_Types...>::is_copy_ctor_noexcept) = default; >+ _Copy_assign_base(_Copy_assign_base&&) >+ noexcept(_Traits<_Types...>::is_move_ctor_noexcept) = default; >+ _Copy_assign_base& operator=(_Copy_assign_base&&) >+ noexcept(_Traits<_Types...>::is_move_assign_noexcept) = default; >+ }; >+ >+ template<typename... _Types> >+ struct _Copy_assign_base<true, _Types...> : _Move_ctor_alias<_Types...> >+ { >+ using _Base = _Move_ctor_alias<_Types...>; >+ using _Base::_Base; >+ }; >+ >+ template<typename... _Types> >+ using _Copy_assign_alias = >+ _Copy_assign_base<_Traits<_Types...>::is_copy_assign_trivial, >+ _Types...>; >+ >+ template<bool, typename... _Types> >+ struct _Move_assign_base : _Copy_assign_alias<_Types...> >+ { >+ using _Base = _Copy_assign_alias<_Types...>; >+ using _Base::_Base; >+ >+ void _M_destructive_move(_Move_assign_base&& __rhs) > { >- this->~_Variant_base(); >+ this->~_Move_assign_base(); > __try > { >- ::new (this) _Variant_base(std::move(__rhs)); >+ ::new (this) _Move_assign_base(std::move(__rhs)); > } > __catch (...) > { >@@ -469,40 +610,81 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > } > } > >- _Variant_base& >- operator=(_Variant_base&& __rhs) >- noexcept((is_nothrow_move_constructible_v<_Types> && ...) >- && (is_nothrow_move_assignable_v<_Types> && ...)) >+ _Move_assign_base& >+ operator=(_Move_assign_base&& __rhs) >+ noexcept(_Traits<_Types...>::is_move_assign_noexcept) > { > if (this->_M_index == __rhs._M_index) > { > if (__rhs._M_valid()) > { > static constexpr void (*_S_vtable[])(void*, void*) = >- { &__erased_assign<_Types&, _Types&&>... }; >- _S_vtable[__rhs._M_index](_M_storage(), __rhs._M_storage()); >+ { &__erased_assign<_Types&, const _Types&>... }; >+ _S_vtable[__rhs._M_index](this->_M_storage(), __rhs._M_storage()); > } > } > else > { >- _M_destructive_move(std::move(__rhs)); >+ _Move_assign_base __tmp(__rhs); >+ this->~_Move_assign_base(); >+ __try >+ { >+ ::new (this) _Move_assign_base(std::move(__tmp)); >+ } >+ __catch (...) >+ { >+ this->_M_index = variant_npos; >+ __throw_exception_again; >+ } > } >+ __glibcxx_assert(this->_M_index == __rhs._M_index); > return *this; > } > >- void* >- _M_storage() const >- { >- return const_cast<void*>(static_cast<const void*>( >- std::addressof(_Storage::_M_u))); >- } >+ _Move_assign_base(const _Move_assign_base& __rhs) >+ noexcept(_Traits<_Types...>::is_copy_ctor_noexcept) = default; >+ _Move_assign_base(_Move_assign_base&&) >+ noexcept(_Traits<_Types...>::is_move_ctor_noexcept) = default; >+ _Move_assign_base& operator=(const _Move_assign_base&) >+ noexcept(_Traits<_Types...>::is_copy_assign_noexcept) = default; >+ }; > >- constexpr bool >- _M_valid() const noexcept >- { >- return this->_M_index != >- typename _Storage::__index_type(variant_npos); >- } >+ template<typename... _Types> >+ struct _Move_assign_base<true, _Types...> : _Copy_assign_alias<_Types...> >+ { >+ using _Base = _Copy_assign_alias<_Types...>; >+ using _Base::_Base; >+ }; >+ >+ template<typename... _Types> >+ using _Move_assign_alias = >+ _Move_assign_base<_Traits<_Types...>::is_move_assign_trivial, >+ _Types...>; >+ >+ template<typename... _Types> >+ struct _Variant_base : _Move_assign_alias<_Types...> >+ { >+ using _Base = _Move_assign_alias<_Types...>; >+ >+ constexpr >+ _Variant_base() >+ noexcept(_Traits<_Types...>::is_default_ctor_noexcept) >+ : _Variant_base(in_place_index<0>) { } >+ >+ template<size_t _Np, typename... _Args> >+ constexpr explicit >+ _Variant_base(in_place_index_t<_Np> __i, _Args&&... __args) >+ : _Base(__i, std::forward<_Args>(__args)...) >+ { } >+ >+ _Variant_base(const _Variant_base& __rhs) >+ noexcept(_Traits<_Types...>::is_copy_ctor_noexcept) = default; >+ _Variant_base(_Variant_base&&) >+ noexcept(_Traits<_Types...>::is_move_ctor_noexcept) = default; >+ _Variant_base& operator=(const _Variant_base&) >+ noexcept(_Traits<_Types...>::is_copy_assign_noexcept) = default; >+ _Variant_base& operator=(_Variant_base&&) >+ noexcept(_Traits<_Types...>::is_move_assign_noexcept) = default; > }; > > // For how many times does _Tp appear in _Tuple? >@@ -877,16 +1059,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > class variant > : private __detail::__variant::_Variant_base<_Types...>, > private _Enable_default_constructor< >- is_default_constructible_v< >- variant_alternative_t<0, variant<_Types...>>>, variant<_Types...>>, >+ __detail::__variant::_Traits<_Types...>::is_default_constructible_v, >+ variant<_Types...>>, > private _Enable_copy_move< >- (is_copy_constructible_v<_Types> && ...), >- (is_copy_constructible_v<_Types> && ...) >- && (is_move_constructible_v<_Types> && ...) >- && (is_copy_assignable_v<_Types> && ...), >- (is_move_constructible_v<_Types> && ...), >- (is_move_constructible_v<_Types> && ...) >- && (is_move_assignable_v<_Types> && ...), >+ __detail::__variant::_Traits<_Types...>::is_copy_constructible_v, >+ __detail::__variant::_Traits<_Types...>::is_copy_assignable_v, >+ __detail::__variant::_Traits<_Types...>::is_move_constructible_v, >+ __detail::__variant::_Traits<_Types...>::is_move_assignable_v, > variant<_Types...>> > { > private: >@@ -899,9 +1078,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > using _Base = __detail::__variant::_Variant_base<_Types...>; > using _Default_ctor_enabler = >- _Enable_default_constructor< >- is_default_constructible_v< >- variant_alternative_t<0, variant<_Types...>>>, variant<_Types...>>; >+ _Enable_default_constructor< >+ __detail::__variant::_Traits<_Types...>::is_default_constructible_v, >+ variant<_Types...>>; > > template<typename _Tp> > static constexpr bool >@@ -928,12 +1107,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > static constexpr size_t __index_of = > __detail::__variant::__index_of_v<_Tp, _Types...>; > >+ using _Traits = __detail::__variant::_Traits<_Types...>; >+ > public: >- constexpr variant() >- noexcept(is_nothrow_default_constructible_v<__to_type<0>>) = default; >- variant(const variant&) = default; >+ variant() noexcept(_Traits::is_default_ctor_noexcept) = default; Do we need the exception specifications here? Will the =default make the right thing happen anyway? (And if not, won't we get an error by trying to define the constructors as noexcept when the implicit definition would not be noexcept?) >+ variant(const variant& __rhs) >+ noexcept(_Traits::is_copy_ctor_noexcept) = default; > variant(variant&&) >- noexcept((is_nothrow_move_constructible_v<_Types> && ...)) = default; >+ noexcept(_Traits::is_move_ctor_noexcept) = default; >+ variant& operator=(const variant&) >+ noexcept(_Traits::is_copy_assign_noexcept) = default; >+ variant& operator=(variant&&) >+ noexcept(_Traits::is_move_assign_noexcept) = default; >+ ~variant() = default; > > template<typename _Tp, > typename = enable_if_t<!is_same_v<decay_t<_Tp>, variant>>, ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch] Forward triviality in variant 2017-06-01 15:13 ` Jonathan Wakely @ 2017-06-01 15:21 ` Ville Voutilainen 2017-06-01 15:29 ` Jonathan Wakely 2017-06-18 19:37 ` Tim Shen via gcc-patches 1 sibling, 1 reply; 11+ messages in thread From: Ville Voutilainen @ 2017-06-01 15:21 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Tim Shen, libstdc++, gcc-patches On 1 June 2017 at 18:13, Jonathan Wakely <jwakely@redhat.com> wrote: > On 30/05/17 02:16 -0700, Tim Shen via libstdc++ wrote: >> >> diff --git a/libstdc++-v3/include/std/variant >> b/libstdc++-v3/include/std/variant >> index b9824a5182c..f81b815af09 100644 >> --- a/libstdc++-v3/include/std/variant >> +++ b/libstdc++-v3/include/std/variant >> @@ -290,6 +290,53 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> __ref_cast<_Tp>(__t)); >> } >> >> + template<typename... _Types> >> + struct _Traits >> + { >> + static constexpr bool is_default_constructible_v = >> + is_default_constructible_v<typename _Nth_type<0, >> _Types...>::type>; >> + static constexpr bool is_copy_constructible_v = >> + __and_<is_copy_constructible<_Types>...>::value; >> + static constexpr bool is_move_constructible_v = >> + __and_<is_move_constructible<_Types>...>::value; >> + static constexpr bool is_copy_assignable_v = >> + is_copy_constructible_v && is_move_constructible_v >> + && __and_<is_copy_assignable<_Types>...>::value; >> + static constexpr bool is_move_assignable_v = >> + is_move_constructible_v >> + && __and_<is_move_assignable<_Types>...>::value; > > > It seems strange to me that these ones end with _v but the following > ones don't. Could we make them all have no _v suffix? Seems to me worth considering to rather make all of them have a _v suffix. :) > >> + static constexpr bool is_dtor_trivial = >> + __and_<is_trivially_destructible<_Types>...>::value; They all seem to be shortcuts for something::value, so it seems to me logical to have them all be _v. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch] Forward triviality in variant 2017-06-01 15:21 ` Ville Voutilainen @ 2017-06-01 15:29 ` Jonathan Wakely 2017-06-01 15:43 ` Ville Voutilainen 0 siblings, 1 reply; 11+ messages in thread From: Jonathan Wakely @ 2017-06-01 15:29 UTC (permalink / raw) To: Ville Voutilainen; +Cc: Tim Shen, libstdc++, gcc-patches On 01/06/17 18:21 +0300, Ville Voutilainen wrote: >On 1 June 2017 at 18:13, Jonathan Wakely <jwakely@redhat.com> wrote: >> On 30/05/17 02:16 -0700, Tim Shen via libstdc++ wrote: >>> >>> diff --git a/libstdc++-v3/include/std/variant >>> b/libstdc++-v3/include/std/variant >>> index b9824a5182c..f81b815af09 100644 >>> --- a/libstdc++-v3/include/std/variant >>> +++ b/libstdc++-v3/include/std/variant >>> @@ -290,6 +290,53 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >>> __ref_cast<_Tp>(__t)); >>> } >>> >>> + template<typename... _Types> >>> + struct _Traits >>> + { >>> + static constexpr bool is_default_constructible_v = >>> + is_default_constructible_v<typename _Nth_type<0, >>> _Types...>::type>; >>> + static constexpr bool is_copy_constructible_v = >>> + __and_<is_copy_constructible<_Types>...>::value; >>> + static constexpr bool is_move_constructible_v = >>> + __and_<is_move_constructible<_Types>...>::value; >>> + static constexpr bool is_copy_assignable_v = >>> + is_copy_constructible_v && is_move_constructible_v >>> + && __and_<is_copy_assignable<_Types>...>::value; >>> + static constexpr bool is_move_assignable_v = >>> + is_move_constructible_v >>> + && __and_<is_move_assignable<_Types>...>::value; >> >> >> It seems strange to me that these ones end with _v but the following >> ones don't. Could we make them all have no _v suffix? > >Seems to me worth considering to rather make all of them have a _v suffix. :) >> >>> + static constexpr bool is_dtor_trivial = >>> + __and_<is_trivially_destructible<_Types>...>::value; > > >They all seem to be shortcuts for something::value, so it seems to me >logical to have >them all be _v. The _v suffixes in the standard are there to distinguish std::foo from std::foo_v, but we don't have that problem. __variant::_Traits<T...>::foo is a unique name, we don't need the suffix, it's just noise. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch] Forward triviality in variant 2017-06-01 15:29 ` Jonathan Wakely @ 2017-06-01 15:43 ` Ville Voutilainen 2017-06-01 16:03 ` Jonathan Wakely 0 siblings, 1 reply; 11+ messages in thread From: Ville Voutilainen @ 2017-06-01 15:43 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Tim Shen, libstdc++, gcc-patches On 1 June 2017 at 18:29, Jonathan Wakely <jwakely@redhat.com> wrote: >> They all seem to be shortcuts for something::value, so it seems to me >> logical to have >> them all be _v. > > > The _v suffixes in the standard are there to distinguish std::foo from > std::foo_v, but we don't have that problem. Wouldn't necessarily hurt to follow the same naming convention idea as the standard, but sure, we don't have that problem, agreed. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch] Forward triviality in variant 2017-06-01 15:43 ` Ville Voutilainen @ 2017-06-01 16:03 ` Jonathan Wakely 2017-06-01 16:07 ` Ville Voutilainen 0 siblings, 1 reply; 11+ messages in thread From: Jonathan Wakely @ 2017-06-01 16:03 UTC (permalink / raw) To: Ville Voutilainen; +Cc: Tim Shen, libstdc++, gcc-patches On 01/06/17 18:43 +0300, Ville Voutilainen wrote: >On 1 June 2017 at 18:29, Jonathan Wakely <jwakely@redhat.com> wrote: >>> They all seem to be shortcuts for something::value, so it seems to me >>> logical to have >>> them all be _v. >> >> >> The _v suffixes in the standard are there to distinguish std::foo from >> std::foo_v, but we don't have that problem. > >Wouldn't necessarily hurt to follow the same naming convention idea as >the standard, but sure, we >don't have that problem, agreed. It's not consistent in the standard: - numeric_limits<T>::is_specialized - std::chrono::system_clock::is_steady - std::atomic<T>::is_always_lock_free And that's OK, because it would be a silly rule that said all boolean constants should end in _v, it would just be noise. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch] Forward triviality in variant 2017-06-01 16:03 ` Jonathan Wakely @ 2017-06-01 16:07 ` Ville Voutilainen 2017-06-01 16:13 ` Jonathan Wakely 0 siblings, 1 reply; 11+ messages in thread From: Ville Voutilainen @ 2017-06-01 16:07 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Tim Shen, libstdc++, gcc-patches On 1 June 2017 at 19:03, Jonathan Wakely <jwakely@redhat.com> wrote: > On 01/06/17 18:43 +0300, Ville Voutilainen wrote: >> >> On 1 June 2017 at 18:29, Jonathan Wakely <jwakely@redhat.com> wrote: >>>> >>>> They all seem to be shortcuts for something::value, so it seems to me >>>> logical to have >>>> them all be _v. >>> The _v suffixes in the standard are there to distinguish std::foo from >>> std::foo_v, but we don't have that problem. >> Wouldn't necessarily hurt to follow the same naming convention idea as >> the standard, but sure, we >> don't have that problem, agreed. > It's not consistent in the standard: > - numeric_limits<T>::is_specialized > - std::chrono::system_clock::is_steady > - std::atomic<T>::is_always_lock_free > > And that's OK, because it would be a silly rule that said all boolean > constants should end in _v, it would just be noise. But I didn't suggest such a rule, merely that if we are doing with a trait-like variable that shortcuts a ::value, then we could entertain using _v. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch] Forward triviality in variant 2017-06-01 16:07 ` Ville Voutilainen @ 2017-06-01 16:13 ` Jonathan Wakely 0 siblings, 0 replies; 11+ messages in thread From: Jonathan Wakely @ 2017-06-01 16:13 UTC (permalink / raw) To: Ville Voutilainen; +Cc: Tim Shen, libstdc++, gcc-patches On 01/06/17 19:07 +0300, Ville Voutilainen wrote: >On 1 June 2017 at 19:03, Jonathan Wakely <jwakely@redhat.com> wrote: >> On 01/06/17 18:43 +0300, Ville Voutilainen wrote: >>> >>> On 1 June 2017 at 18:29, Jonathan Wakely <jwakely@redhat.com> wrote: >>>>> >>>>> They all seem to be shortcuts for something::value, so it seems to me >>>>> logical to have >>>>> them all be _v. >>>> The _v suffixes in the standard are there to distinguish std::foo from >>>> std::foo_v, but we don't have that problem. >>> Wouldn't necessarily hurt to follow the same naming convention idea as >>> the standard, but sure, we >>> don't have that problem, agreed. >> It's not consistent in the standard: >> - numeric_limits<T>::is_specialized >> - std::chrono::system_clock::is_steady >> - std::atomic<T>::is_always_lock_free >> >> And that's OK, because it would be a silly rule that said all boolean >> constants should end in _v, it would just be noise. > > >But I didn't suggest such a rule, merely that if we are doing with a >trait-like variable >that shortcuts a ::value, then we could entertain using _v. The trait describes properties of the variant. The fact those properties are determined by something::value is an implementation detail, not an important feature that needs to be in the name. The implementation details should not leak into the public API of the trait. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch] Forward triviality in variant 2017-06-01 15:13 ` Jonathan Wakely 2017-06-01 15:21 ` Ville Voutilainen @ 2017-06-18 19:37 ` Tim Shen via gcc-patches 2017-06-27 15:43 ` Jonathan Wakely 1 sibling, 1 reply; 11+ messages in thread From: Tim Shen via gcc-patches @ 2017-06-18 19:37 UTC (permalink / raw) To: Jonathan Wakely, Ville Voutilainen; +Cc: libstdc++, gcc-patches [-- Attachment #1: Type: text/plain, Size: 4661 bytes --] Besides the changes on the comments, I also changed the definition of _S_trivial_copy_assign and _S_trivial_move_assign to match what union has. See [class.copy.assign]p9. On Thu, Jun 1, 2017 at 8:13 AM, Jonathan Wakely wrote: > On 30/05/17 02:16 -0700, Tim Shen via libstdc++ wrote: >> >> diff --git a/libstdc++-v3/include/std/variant >> b/libstdc++-v3/include/std/variant >> index b9824a5182c..f81b815af09 100644 >> --- a/libstdc++-v3/include/std/variant >> +++ b/libstdc++-v3/include/std/variant >> @@ -290,6 +290,53 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> __ref_cast<_Tp>(__t)); >> } >> >> + template<typename... _Types> >> + struct _Traits >> + { >> + static constexpr bool is_default_constructible_v = >> + is_default_constructible_v<typename _Nth_type<0, >> _Types...>::type>; >> + static constexpr bool is_copy_constructible_v = >> + __and_<is_copy_constructible<_Types>...>::value; >> + static constexpr bool is_move_constructible_v = >> + __and_<is_move_constructible<_Types>...>::value; >> + static constexpr bool is_copy_assignable_v = >> + is_copy_constructible_v && is_move_constructible_v >> + && __and_<is_copy_assignable<_Types>...>::value; >> + static constexpr bool is_move_assignable_v = >> + is_move_constructible_v >> + && __and_<is_move_assignable<_Types>...>::value; > > > It seems strange to me that these ones end with _v but the following > ones don't. Could we make them all have no _v suffix? Done. They are internal traits only for readability, so I shortened the names and make them libstdc++ style, e.g. _S_copy_ctor. > >> + static constexpr bool is_dtor_trivial = >> + __and_<is_trivially_destructible<_Types>...>::value; >> + static constexpr bool is_copy_ctor_trivial = >> + __and_<is_trivially_copy_constructible<_Types>...>::value; >> + static constexpr bool is_move_ctor_trivial = >> + __and_<is_trivially_move_constructible<_Types>...>::value; >> + static constexpr bool is_copy_assign_trivial = >> + is_dtor_trivial >> + && is_copy_ctor_trivial >> + && __and_<is_trivially_copy_assignable<_Types>...>::value; >> + static constexpr bool is_move_assign_trivial = >> + is_dtor_trivial >> + && is_move_ctor_trivial >> + && __and_<is_trivially_move_assignable<_Types>...>::value; >> + >> + static constexpr bool is_default_ctor_noexcept = >> + is_nothrow_default_constructible_v< >> + typename _Nth_type<0, _Types...>::type>; >> + static constexpr bool is_copy_ctor_noexcept = >> + is_copy_ctor_trivial; >> + static constexpr bool is_move_ctor_noexcept = >> + is_move_ctor_trivial >> + || __and_<is_nothrow_move_constructible<_Types>...>::value; >> + static constexpr bool is_copy_assign_noexcept = >> + is_copy_assign_trivial; >> + static constexpr bool is_move_assign_noexcept = >> + is_move_assign_trivial || >> + (is_move_ctor_noexcept >> + && __and_<is_nothrow_move_assignable<_Types>...>::value); >> + }; > > > Does using __and_ for any of those traits reduce the limit on the > number of alternatives in a variant? We switched to using fold > expressions in some contexts to avoid very deep instantiations, but I > don't know if these will hit the same problem, but it looks like it > will. Done, use fold expression instead. At one point we changed some fold expressions to __and_, because __and_ has short circuiting; does fold expressions have short circuits too? Now that I think about it, short circuiting in a constant fold expression should be a QoI issue. >> @@ -928,12 +1107,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> static constexpr size_t __index_of = >> __detail::__variant::__index_of_v<_Tp, _Types...>; >> >> + using _Traits = __detail::__variant::_Traits<_Types...>; >> + >> public: >> - constexpr variant() >> - noexcept(is_nothrow_default_constructible_v<__to_type<0>>) = >> default; >> - variant(const variant&) = default; >> + variant() noexcept(_Traits::is_default_ctor_noexcept) = default; > > > Do we need the exception specifications here? Will the =default make > the right thing happen anyway? (And if not, won't we get an error by > trying to define the constructors as noexcept when the implicit > definition would not be noexcept?) Done. Removed unnecessary noexcept qualifiers. It turns out I mistakenly thought using "variant() = default" means `variant() noexcept(false) = default`. -- Regards, Tim Shen [-- Attachment #2: c.diff --] [-- Type: text/x-patch, Size: 24736 bytes --] commit 919492daf1c5ff78de4a33bdae265ea1acacc446 Author: Tim Shen <timshen@google.com> Date: Mon May 29 22:44:42 2017 -0700 2017-05-30 Tim Shen <timshen@google.com> PR libstdc++/80187 * include/std/variant (variant::variant, variant::~variant, variant::operator=): Implement triviality forwarding for four special member functions. * testsuite/20_util/variant/compile.cc: Tests. diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant index b9824a5182c..ef0cba98388 100644 --- a/libstdc++-v3/include/std/variant +++ b/libstdc++-v3/include/std/variant @@ -290,6 +290,46 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __ref_cast<_Tp>(__t)); } + template<typename... _Types> + struct _Traits + { + static constexpr bool _S_default_ctor = + is_default_constructible_v<typename _Nth_type<0, _Types...>::type>; + static constexpr bool _S_copy_ctor = + (is_copy_constructible_v<_Types> && ...); + static constexpr bool _S_move_ctor = + (is_move_constructible_v<_Types> && ...); + static constexpr bool _S_copy_assign = + _S_copy_ctor && _S_move_ctor + && (is_copy_assignable_v<_Types> && ...); + static constexpr bool _S_move_assign = + _S_move_ctor + && (is_move_assignable_v<_Types> && ...); + + static constexpr bool _S_trivial_dtor = + (is_trivially_destructible_v<_Types> && ...); + static constexpr bool _S_trivial_copy_ctor = + (is_trivially_copy_constructible_v<_Types> && ...); + static constexpr bool _S_trivial_move_ctor = + (is_trivially_move_constructible_v<_Types> && ...); + static constexpr bool _S_trivial_copy_assign = + _S_trivial_dtor && (is_trivially_copy_assignable_v<_Types> && ...); + static constexpr bool _S_trivial_move_assign = + _S_trivial_dtor && (is_trivially_move_assignable_v<_Types> && ...); + + // The following nothrow traits are for non-trivial SMFs. Trivial SMFs + // are always nothrow. + static constexpr bool _S_nothrow_default_ctor = + is_nothrow_default_constructible_v< + typename _Nth_type<0, _Types...>::type>; + static constexpr bool _S_nothrow_copy_ctor = false; + static constexpr bool _S_nothrow_move_ctor = + (is_nothrow_move_constructible_v<_Types> && ...); + static constexpr bool _S_nothrow_copy_assign = false; + static constexpr bool _S_nothrow_move_assign = + _S_nothrow_move_ctor && (is_nothrow_move_assignable_v<_Types> && ...); + }; + // Defines members and ctors. template<typename... _Types> union _Variadic_union { }; @@ -355,6 +395,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION ~_Variant_storage() { _M_reset(); } + void* + _M_storage() const + { + return const_cast<void*>(static_cast<const void*>( + std::addressof(_M_u))); + } + + constexpr bool + _M_valid() const noexcept + { + return this->_M_index != __index_type(variant_npos); + } + _Variadic_union<_Types...> _M_u; using __index_type = __select_index<_Types...>; __index_type _M_index; @@ -374,59 +427,108 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION void _M_reset() { _M_index = variant_npos; } + void* + _M_storage() const + { + return const_cast<void*>(static_cast<const void*>( + std::addressof(_M_u))); + } + + constexpr bool + _M_valid() const noexcept + { + return this->_M_index != __index_type(variant_npos); + } + _Variadic_union<_Types...> _M_u; using __index_type = __select_index<_Types...>; __index_type _M_index; }; - // Helps SFINAE on special member functions. Otherwise it can live in variant - // class. template<typename... _Types> - struct _Variant_base : - _Variant_storage<(std::is_trivially_destructible_v<_Types> && ...), - _Types...> - { - using _Storage = - _Variant_storage<(std::is_trivially_destructible_v<_Types> && ...), - _Types...>; + using _Variant_storage_alias = + _Variant_storage<_Traits<_Types...>::_S_trivial_dtor, _Types...>; - constexpr - _Variant_base() - noexcept(is_nothrow_default_constructible_v< - variant_alternative_t<0, variant<_Types...>>>) - : _Variant_base(in_place_index<0>) { } + // The following are (Copy|Move) (ctor|assign) layers for forwarding + // triviality and handling non-trivial SMF behaviors. + + template<bool, typename... _Types> + struct _Copy_ctor_base : _Variant_storage_alias<_Types...> + { + using _Base = _Variant_storage_alias<_Types...>; + using _Base::_Base; - _Variant_base(const _Variant_base& __rhs) + _Copy_ctor_base(const _Copy_ctor_base& __rhs) + noexcept(_Traits<_Types...>::_S_nothrow_copy_ctor) { if (__rhs._M_valid()) { static constexpr void (*_S_vtable[])(void*, void*) = { &__erased_ctor<_Types&, const _Types&>... }; - _S_vtable[__rhs._M_index](_M_storage(), __rhs._M_storage()); + _S_vtable[__rhs._M_index](this->_M_storage(), __rhs._M_storage()); this->_M_index = __rhs._M_index; } } - _Variant_base(_Variant_base&& __rhs) - noexcept((is_nothrow_move_constructible_v<_Types> && ...)) + _Copy_ctor_base(_Copy_ctor_base&&) = default; + _Copy_ctor_base& operator=(const _Copy_ctor_base&) = default; + _Copy_ctor_base& operator=(_Copy_ctor_base&&) = default; + }; + + template<typename... _Types> + struct _Copy_ctor_base<true, _Types...> : _Variant_storage_alias<_Types...> + { + using _Base = _Variant_storage_alias<_Types...>; + using _Base::_Base; + }; + + template<typename... _Types> + using _Copy_ctor_alias = + _Copy_ctor_base<_Traits<_Types...>::_S_trivial_copy_ctor, _Types...>; + + template<bool, typename... _Types> + struct _Move_ctor_base : _Copy_ctor_alias<_Types...> + { + using _Base = _Copy_ctor_alias<_Types...>; + using _Base::_Base; + + _Move_ctor_base(_Move_ctor_base&& __rhs) + noexcept(_Traits<_Types...>::_S_nothrow_move_ctor) { if (__rhs._M_valid()) { static constexpr void (*_S_vtable[])(void*, void*) = { &__erased_ctor<_Types&, _Types&&>... }; - _S_vtable[__rhs._M_index](_M_storage(), __rhs._M_storage()); + _S_vtable[__rhs._M_index](this->_M_storage(), __rhs._M_storage()); this->_M_index = __rhs._M_index; } } - template<size_t _Np, typename... _Args> - constexpr explicit - _Variant_base(in_place_index_t<_Np> __i, _Args&&... __args) - : _Storage(__i, std::forward<_Args>(__args)...) - { } + _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; + }; + + template<typename... _Types> + struct _Move_ctor_base<true, _Types...> : _Copy_ctor_alias<_Types...> + { + using _Base = _Copy_ctor_alias<_Types...>; + using _Base::_Base; + }; + + template<typename... _Types> + using _Move_ctor_alias = + _Move_ctor_base<_Traits<_Types...>::_S_trivial_move_ctor, _Types...>; + + template<bool, typename... _Types> + struct _Copy_assign_base : _Move_ctor_alias<_Types...> + { + using _Base = _Move_ctor_alias<_Types...>; + using _Base::_Base; - _Variant_base& - operator=(const _Variant_base& __rhs) + _Copy_assign_base& + operator=(const _Copy_assign_base& __rhs) + noexcept(_Traits<_Types...>::_S_nothrow_copy_assign) { if (this->_M_index == __rhs._M_index) { @@ -434,16 +536,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { static constexpr void (*_S_vtable[])(void*, void*) = { &__erased_assign<_Types&, const _Types&>... }; - _S_vtable[__rhs._M_index](_M_storage(), __rhs._M_storage()); + _S_vtable[__rhs._M_index](this->_M_storage(), __rhs._M_storage()); } } else { - _Variant_base __tmp(__rhs); - this->~_Variant_base(); + _Copy_assign_base __tmp(__rhs); + this->~_Copy_assign_base(); __try { - ::new (this) _Variant_base(std::move(__tmp)); + ::new (this) _Copy_assign_base(std::move(__tmp)); } __catch (...) { @@ -455,12 +557,35 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION return *this; } - void _M_destructive_move(_Variant_base&& __rhs) + _Copy_assign_base(const _Copy_assign_base&) = default; + _Copy_assign_base(_Copy_assign_base&&) = default; + _Copy_assign_base& operator=(_Copy_assign_base&&) = default; + }; + + template<typename... _Types> + struct _Copy_assign_base<true, _Types...> : _Move_ctor_alias<_Types...> + { + using _Base = _Move_ctor_alias<_Types...>; + using _Base::_Base; + }; + + template<typename... _Types> + using _Copy_assign_alias = + _Copy_assign_base<_Traits<_Types...>::_S_trivial_copy_assign, + _Types...>; + + template<bool, typename... _Types> + struct _Move_assign_base : _Copy_assign_alias<_Types...> + { + using _Base = _Copy_assign_alias<_Types...>; + using _Base::_Base; + + void _M_destructive_move(_Move_assign_base&& __rhs) { - this->~_Variant_base(); + this->~_Move_assign_base(); __try { - ::new (this) _Variant_base(std::move(__rhs)); + ::new (this) _Move_assign_base(std::move(__rhs)); } __catch (...) { @@ -469,40 +594,74 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } } - _Variant_base& - operator=(_Variant_base&& __rhs) - noexcept((is_nothrow_move_constructible_v<_Types> && ...) - && (is_nothrow_move_assignable_v<_Types> && ...)) + _Move_assign_base& + operator=(_Move_assign_base&& __rhs) + noexcept(_Traits<_Types...>::_S_nothrow_move_assign) { if (this->_M_index == __rhs._M_index) { if (__rhs._M_valid()) { static constexpr void (*_S_vtable[])(void*, void*) = - { &__erased_assign<_Types&, _Types&&>... }; - _S_vtable[__rhs._M_index](_M_storage(), __rhs._M_storage()); + { &__erased_assign<_Types&, const _Types&>... }; + _S_vtable[__rhs._M_index](this->_M_storage(), __rhs._M_storage()); } } else { - _M_destructive_move(std::move(__rhs)); + _Move_assign_base __tmp(__rhs); + this->~_Move_assign_base(); + __try + { + ::new (this) _Move_assign_base(std::move(__tmp)); + } + __catch (...) + { + this->_M_index = variant_npos; + __throw_exception_again; + } } + __glibcxx_assert(this->_M_index == __rhs._M_index); return *this; } - void* - _M_storage() const - { - return const_cast<void*>(static_cast<const void*>( - std::addressof(_Storage::_M_u))); - } + _Move_assign_base(const _Move_assign_base&) = default; + _Move_assign_base(_Move_assign_base&&) = default; + _Move_assign_base& operator=(const _Move_assign_base&) = default; + }; - constexpr bool - _M_valid() const noexcept - { - return this->_M_index != - typename _Storage::__index_type(variant_npos); - } + template<typename... _Types> + struct _Move_assign_base<true, _Types...> : _Copy_assign_alias<_Types...> + { + using _Base = _Copy_assign_alias<_Types...>; + using _Base::_Base; + }; + + template<typename... _Types> + using _Move_assign_alias = + _Move_assign_base<_Traits<_Types...>::_S_trivial_move_assign, + _Types...>; + + template<typename... _Types> + struct _Variant_base : _Move_assign_alias<_Types...> + { + using _Base = _Move_assign_alias<_Types...>; + + constexpr + _Variant_base() + noexcept(_Traits<_Types...>::_S_nothrow_default_ctor) + : _Variant_base(in_place_index<0>) { } + + template<size_t _Np, typename... _Args> + constexpr explicit + _Variant_base(in_place_index_t<_Np> __i, _Args&&... __args) + : _Base(__i, std::forward<_Args>(__args)...) + { } + + _Variant_base(const _Variant_base&) = default; + _Variant_base(_Variant_base&&) = default; + _Variant_base& operator=(const _Variant_base&) = default; + _Variant_base& operator=(_Variant_base&&) = default; }; // For how many times does _Tp appear in _Tuple? @@ -877,16 +1036,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION class variant : private __detail::__variant::_Variant_base<_Types...>, private _Enable_default_constructor< - is_default_constructible_v< - variant_alternative_t<0, variant<_Types...>>>, variant<_Types...>>, + __detail::__variant::_Traits<_Types...>::_S_default_ctor, + variant<_Types...>>, private _Enable_copy_move< - (is_copy_constructible_v<_Types> && ...), - (is_copy_constructible_v<_Types> && ...) - && (is_move_constructible_v<_Types> && ...) - && (is_copy_assignable_v<_Types> && ...), - (is_move_constructible_v<_Types> && ...), - (is_move_constructible_v<_Types> && ...) - && (is_move_assignable_v<_Types> && ...), + __detail::__variant::_Traits<_Types...>::_S_copy_ctor, + __detail::__variant::_Traits<_Types...>::_S_copy_assign, + __detail::__variant::_Traits<_Types...>::_S_move_ctor, + __detail::__variant::_Traits<_Types...>::_S_move_assign, variant<_Types...>> { private: @@ -899,9 +1055,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION using _Base = __detail::__variant::_Variant_base<_Types...>; using _Default_ctor_enabler = - _Enable_default_constructor< - is_default_constructible_v< - variant_alternative_t<0, variant<_Types...>>>, variant<_Types...>>; + _Enable_default_constructor< + __detail::__variant::_Traits<_Types...>::_S_default_ctor, + variant<_Types...>>; template<typename _Tp> static constexpr bool @@ -928,12 +1084,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION static constexpr size_t __index_of = __detail::__variant::__index_of_v<_Tp, _Types...>; + using _Traits = __detail::__variant::_Traits<_Types...>; + public: - constexpr variant() - noexcept(is_nothrow_default_constructible_v<__to_type<0>>) = default; - variant(const variant&) = default; - variant(variant&&) - noexcept((is_nothrow_move_constructible_v<_Types> && ...)) = default; + variant() = default; + variant(const variant& __rhs) = default; + variant(variant&&) = default; + variant& operator=(const variant&) = default; + variant& operator=(variant&&) = default; + ~variant() = default; template<typename _Tp, typename = enable_if_t<!is_same_v<decay_t<_Tp>, variant>>, @@ -942,7 +1101,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION constexpr variant(_Tp&& __t) noexcept(is_nothrow_constructible_v<__accepted_type<_Tp&&>, _Tp&&>) - : variant(in_place_index<__accepted_index<_Tp&&>>, std::forward<_Tp>(__t)) + : variant(in_place_index<__accepted_index<_Tp&&>>, + std::forward<_Tp>(__t)) { __glibcxx_assert(holds_alternative<__accepted_type<_Tp&&>>(*this)); } template<typename _Tp, typename... _Args, @@ -950,7 +1110,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION && is_constructible_v<_Tp, _Args&&...>>> constexpr explicit variant(in_place_type_t<_Tp>, _Args&&... __args) - : variant(in_place_index<__index_of<_Tp>>, std::forward<_Args>(__args)...) + : variant(in_place_index<__index_of<_Tp>>, + std::forward<_Args>(__args)...) { __glibcxx_assert(holds_alternative<_Tp>(*this)); } template<typename _Tp, typename _Up, typename... _Args, @@ -983,13 +1144,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _Default_ctor_enabler(_Enable_default_constructor_tag{}) { __glibcxx_assert(index() == _Np); } - ~variant() = default; - - variant& operator=(const variant&) = default; - variant& operator=(variant&&) - noexcept((is_nothrow_move_constructible_v<_Types> && ...) - && (is_nothrow_move_assignable_v<_Types> && ...)) = default; - template<typename _Tp> enable_if_t<__exactly_once<__accepted_type<_Tp&&>> && is_constructible_v<__accepted_type<_Tp&&>, _Tp&&> @@ -1084,7 +1238,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION constexpr size_t index() const noexcept { if (this->_M_index == - typename _Base::_Storage::__index_type(variant_npos)) + typename _Base::__index_type(variant_npos)) return variant_npos; return this->_M_index; } diff --git a/libstdc++-v3/testsuite/20_util/variant/compile.cc b/libstdc++-v3/testsuite/20_util/variant/compile.cc index 06e8eb31ee8..e5f7538ba42 100644 --- a/libstdc++-v3/testsuite/20_util/variant/compile.cc +++ b/libstdc++-v3/testsuite/20_util/variant/compile.cc @@ -88,10 +88,12 @@ void copy_ctor() { static_assert(is_copy_constructible_v<variant<int, string>>, ""); static_assert(!is_copy_constructible_v<variant<AllDeleted, string>>, ""); + static_assert(is_trivially_copy_constructible_v<variant<int>>, ""); + static_assert(!is_trivially_copy_constructible_v<variant<std::string>>, ""); { variant<int> a; - static_assert(!noexcept(variant<int>(a)), ""); + static_assert(noexcept(variant<int>(a)), ""); } { variant<string> a; @@ -103,7 +105,7 @@ void copy_ctor() } { variant<int, char> a; - static_assert(!noexcept(variant<int, char>(a)), ""); + static_assert(noexcept(variant<int, char>(a)), ""); } } @@ -111,6 +113,8 @@ void move_ctor() { static_assert(is_move_constructible_v<variant<int, string>>, ""); static_assert(!is_move_constructible_v<variant<AllDeleted, string>>, ""); + static_assert(is_trivially_move_constructible_v<variant<int>>, ""); + static_assert(!is_trivially_move_constructible_v<variant<std::string>>, ""); static_assert(!noexcept(variant<int, Empty>(declval<variant<int, Empty>>())), ""); static_assert(noexcept(variant<int, DefaultNoexcept>(declval<variant<int, DefaultNoexcept>>())), ""); } @@ -148,13 +152,15 @@ void copy_assign() { static_assert(is_copy_assignable_v<variant<int, string>>, ""); static_assert(!is_copy_assignable_v<variant<AllDeleted, string>>, ""); + static_assert(is_trivially_copy_assignable_v<variant<int>>, ""); + static_assert(!is_trivially_copy_assignable_v<variant<string>>, ""); { variant<Empty> a; static_assert(!noexcept(a = a), ""); } { variant<DefaultNoexcept> a; - static_assert(!noexcept(a = a), ""); + static_assert(noexcept(a = a), ""); } } @@ -162,6 +168,8 @@ void move_assign() { static_assert(is_move_assignable_v<variant<int, string>>, ""); static_assert(!is_move_assignable_v<variant<AllDeleted, string>>, ""); + static_assert(is_trivially_move_assignable_v<variant<int>>, ""); + static_assert(!is_trivially_move_assignable_v<variant<string>>, ""); { variant<Empty> a; static_assert(!noexcept(a = std::move(a)), ""); @@ -454,3 +462,92 @@ void test_emplace() static_assert(!has_type_emplace<variant<AllDeleted>, AllDeleted>(0), ""); static_assert(!has_index_emplace<variant<AllDeleted>, 0>(0), ""); } + +void test_triviality() +{ +#define TEST_TEMPLATE(DT, CC, MC, CA, MA, CC_VAL, MC_VAL, CA_VAL, MA_VAL) \ + { \ + struct A \ + { \ + ~A() DT; \ + A(const A&) CC; \ + A(A&&) MC; \ + A& operator=(const A&) CA; \ + A& operator=(A&&) MA; \ + }; \ + static_assert(CC_VAL == is_trivially_copy_constructible_v<variant<A>>, ""); \ + static_assert(MC_VAL == is_trivially_move_constructible_v<variant<A>>, ""); \ + static_assert(CA_VAL == is_trivially_copy_assignable_v<variant<A>>, ""); \ + static_assert(MA_VAL == is_trivially_move_assignable_v<variant<A>>, ""); \ + } + TEST_TEMPLATE(=default, =default, =default, =default, =default, true, true, true, true) + TEST_TEMPLATE(=default, =default, =default, =default, {}, true, true, true, false) + TEST_TEMPLATE(=default, =default, =default, {}, =default, true, true, false, true) + TEST_TEMPLATE(=default, =default, =default, {}, {}, true, true, false, false) + TEST_TEMPLATE(=default, =default, {}, =default, =default, true, false, true, true) + TEST_TEMPLATE(=default, =default, {}, =default, {}, true, false, true, false) + TEST_TEMPLATE(=default, =default, {}, {}, =default, true, false, false, true) + TEST_TEMPLATE(=default, =default, {}, {}, {}, true, false, false, false) + TEST_TEMPLATE(=default, {}, =default, =default, =default, false, true, true, true) + TEST_TEMPLATE(=default, {}, =default, =default, {}, false, true, true, false) + TEST_TEMPLATE(=default, {}, =default, {}, =default, false, true, false, true) + TEST_TEMPLATE(=default, {}, =default, {}, {}, false, true, false, false) + TEST_TEMPLATE(=default, {}, {}, =default, =default, false, false, true, true) + TEST_TEMPLATE(=default, {}, {}, =default, {}, false, false, true, false) + TEST_TEMPLATE(=default, {}, {}, {}, =default, false, false, false, true) + TEST_TEMPLATE(=default, {}, {}, {}, {}, false, false, false, false) + TEST_TEMPLATE( {}, =default, =default, =default, =default, false, false, false, false) + TEST_TEMPLATE( {}, =default, =default, =default, {}, false, false, false, false) + TEST_TEMPLATE( {}, =default, =default, {}, =default, false, false, false, false) + TEST_TEMPLATE( {}, =default, =default, {}, {}, false, false, false, false) + TEST_TEMPLATE( {}, =default, {}, =default, =default, false, false, false, false) + TEST_TEMPLATE( {}, =default, {}, =default, {}, false, false, false, false) + TEST_TEMPLATE( {}, =default, {}, {}, =default, false, false, false, false) + TEST_TEMPLATE( {}, =default, {}, {}, {}, false, false, false, false) + TEST_TEMPLATE( {}, {}, =default, =default, =default, false, false, false, false) + TEST_TEMPLATE( {}, {}, =default, =default, {}, false, false, false, false) + TEST_TEMPLATE( {}, {}, =default, {}, =default, false, false, false, false) + TEST_TEMPLATE( {}, {}, =default, {}, {}, false, false, false, false) + TEST_TEMPLATE( {}, {}, {}, =default, =default, false, false, false, false) + TEST_TEMPLATE( {}, {}, {}, =default, {}, false, false, false, false) + TEST_TEMPLATE( {}, {}, {}, {}, =default, false, false, false, false) + TEST_TEMPLATE( {}, {}, {}, {}, {}, false, false, false, false) +#undef TEST_TEMPLATE + +#define TEST_TEMPLATE(CC, MC, CA, MA) \ + { \ + struct A \ + { \ + A(const A&) CC; \ + A(A&&) MC; \ + A& operator=(const A&) CA; \ + A& operator=(A&&) MA; \ + }; \ + static_assert(!is_trivially_copy_constructible_v<variant<AllDeleted, A>>, ""); \ + static_assert(!is_trivially_move_constructible_v<variant<AllDeleted, A>>, ""); \ + static_assert(!is_trivially_copy_assignable_v<variant<AllDeleted, A>>, ""); \ + static_assert(!is_trivially_move_assignable_v<variant<AllDeleted, A>>, ""); \ + } + TEST_TEMPLATE(=default, =default, =default, =default) + TEST_TEMPLATE(=default, =default, =default, {}) + TEST_TEMPLATE(=default, =default, {}, =default) + TEST_TEMPLATE(=default, =default, {}, {}) + TEST_TEMPLATE(=default, {}, =default, =default) + TEST_TEMPLATE(=default, {}, =default, {}) + TEST_TEMPLATE(=default, {}, {}, =default) + TEST_TEMPLATE(=default, {}, {}, {}) + TEST_TEMPLATE( {}, =default, =default, =default) + TEST_TEMPLATE( {}, =default, =default, {}) + TEST_TEMPLATE( {}, =default, {}, =default) + TEST_TEMPLATE( {}, =default, {}, {}) + TEST_TEMPLATE( {}, {}, =default, =default) + TEST_TEMPLATE( {}, {}, =default, {}) + TEST_TEMPLATE( {}, {}, {}, =default) + TEST_TEMPLATE( {}, {}, {}, {}) +#undef TEST_TEMPLATE + + static_assert(is_trivially_copy_constructible_v<variant<DefaultNoexcept, int, char, float, double>>, ""); + static_assert(is_trivially_move_constructible_v<variant<DefaultNoexcept, int, char, float, double>>, ""); + static_assert(is_trivially_copy_assignable_v<variant<DefaultNoexcept, int, char, float, double>>, ""); + static_assert(is_trivially_move_assignable_v<variant<DefaultNoexcept, int, char, float, double>>, ""); +} ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch] Forward triviality in variant 2017-06-18 19:37 ` Tim Shen via gcc-patches @ 2017-06-27 15:43 ` Jonathan Wakely 0 siblings, 0 replies; 11+ messages in thread From: Jonathan Wakely @ 2017-06-27 15:43 UTC (permalink / raw) To: Tim Shen; +Cc: Ville Voutilainen, libstdc++, gcc-patches On 18/06/17 12:37 -0700, Tim Shen via libstdc++ wrote: >Besides the changes on the comments, I also changed the definition of >_S_trivial_copy_assign and _S_trivial_move_assign to match what union >has. See [class.copy.assign]p9. > >On Thu, Jun 1, 2017 at 8:13 AM, Jonathan Wakely wrote: >> On 30/05/17 02:16 -0700, Tim Shen via libstdc++ wrote: >>> >>> diff --git a/libstdc++-v3/include/std/variant >>> b/libstdc++-v3/include/std/variant >>> index b9824a5182c..f81b815af09 100644 >>> --- a/libstdc++-v3/include/std/variant >>> +++ b/libstdc++-v3/include/std/variant >>> @@ -290,6 +290,53 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >>> __ref_cast<_Tp>(__t)); >>> } >>> >>> + template<typename... _Types> >>> + struct _Traits >>> + { >>> + static constexpr bool is_default_constructible_v = >>> + is_default_constructible_v<typename _Nth_type<0, >>> _Types...>::type>; >>> + static constexpr bool is_copy_constructible_v = >>> + __and_<is_copy_constructible<_Types>...>::value; >>> + static constexpr bool is_move_constructible_v = >>> + __and_<is_move_constructible<_Types>...>::value; >>> + static constexpr bool is_copy_assignable_v = >>> + is_copy_constructible_v && is_move_constructible_v >>> + && __and_<is_copy_assignable<_Types>...>::value; >>> + static constexpr bool is_move_assignable_v = >>> + is_move_constructible_v >>> + && __and_<is_move_assignable<_Types>...>::value; >> >> >> It seems strange to me that these ones end with _v but the following >> ones don't. Could we make them all have no _v suffix? > >Done. They are internal traits only for readability, so I shortened >the names and make them libstdc++ style, e.g. _S_copy_ctor. > >> >>> + static constexpr bool is_dtor_trivial = >>> + __and_<is_trivially_destructible<_Types>...>::value; >>> + static constexpr bool is_copy_ctor_trivial = >>> + __and_<is_trivially_copy_constructible<_Types>...>::value; >>> + static constexpr bool is_move_ctor_trivial = >>> + __and_<is_trivially_move_constructible<_Types>...>::value; >>> + static constexpr bool is_copy_assign_trivial = >>> + is_dtor_trivial >>> + && is_copy_ctor_trivial >>> + && __and_<is_trivially_copy_assignable<_Types>...>::value; >>> + static constexpr bool is_move_assign_trivial = >>> + is_dtor_trivial >>> + && is_move_ctor_trivial >>> + && __and_<is_trivially_move_assignable<_Types>...>::value; >>> + >>> + static constexpr bool is_default_ctor_noexcept = >>> + is_nothrow_default_constructible_v< >>> + typename _Nth_type<0, _Types...>::type>; >>> + static constexpr bool is_copy_ctor_noexcept = >>> + is_copy_ctor_trivial; >>> + static constexpr bool is_move_ctor_noexcept = >>> + is_move_ctor_trivial >>> + || __and_<is_nothrow_move_constructible<_Types>...>::value; >>> + static constexpr bool is_copy_assign_noexcept = >>> + is_copy_assign_trivial; >>> + static constexpr bool is_move_assign_noexcept = >>> + is_move_assign_trivial || >>> + (is_move_ctor_noexcept >>> + && __and_<is_nothrow_move_assignable<_Types>...>::value); >>> + }; >> >> >> Does using __and_ for any of those traits reduce the limit on the >> number of alternatives in a variant? We switched to using fold >> expressions in some contexts to avoid very deep instantiations, but I >> don't know if these will hit the same problem, but it looks like it >> will. > >Done, use fold expression instead. At one point we changed some fold >expressions to __and_, because __and_ has short circuiting; does fold >expressions have short circuits too? Now that I think about it, short >circuiting in a constant fold expression should be a QoI issue. Fold expressions don't short-circuit ... I'm not sure if they would be allowed to for QoI. >>> @@ -928,12 +1107,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >>> static constexpr size_t __index_of = >>> __detail::__variant::__index_of_v<_Tp, _Types...>; >>> >>> + using _Traits = __detail::__variant::_Traits<_Types...>; >>> + >>> public: >>> - constexpr variant() >>> - noexcept(is_nothrow_default_constructible_v<__to_type<0>>) = >>> default; >>> - variant(const variant&) = default; >>> + variant() noexcept(_Traits::is_default_ctor_noexcept) = default; >> >> >> Do we need the exception specifications here? Will the =default make >> the right thing happen anyway? (And if not, won't we get an error by >> trying to define the constructors as noexcept when the implicit >> definition would not be noexcept?) > >Done. Removed unnecessary noexcept qualifiers. > >It turns out I mistakenly thought using "variant() = default" means >`variant() noexcept(false) = default`. OK for trunk, thanks. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-06-27 15:43 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-30 6:36 [Patch] Forward triviality in variant Tim Shen via gcc-patches 2017-05-30 9:41 ` Tim Shen via gcc-patches 2017-06-01 15:13 ` Jonathan Wakely 2017-06-01 15:21 ` Ville Voutilainen 2017-06-01 15:29 ` Jonathan Wakely 2017-06-01 15:43 ` Ville Voutilainen 2017-06-01 16:03 ` Jonathan Wakely 2017-06-01 16:07 ` Ville Voutilainen 2017-06-01 16:13 ` Jonathan Wakely 2017-06-18 19:37 ` Tim Shen via gcc-patches 2017-06-27 15:43 ` Jonathan Wakely
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).