From: Jonathan Wakely <jwakely@redhat.com>
To: Tim Shen <timshen@google.com>
Cc: libstdc++ <libstdc++@gcc.gnu.org>, gcc-patches <gcc-patches@gcc.gnu.org>
Subject: Re: [Patch] Forward triviality in variant
Date: Thu, 01 Jun 2017 15:13:00 -0000 [thread overview]
Message-ID: <20170601151302.GA10443@redhat.com> (raw)
In-Reply-To: <CAG4ZjNm1vWNAS8qytZaFV7DDdS3=fONkqjgUWUA4cY0ppsuaNg@mail.gmail.com>
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>>,
next prev parent reply other threads:[~2017-06-01 15:13 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-30 6:36 Tim Shen via gcc-patches
2017-05-30 9:41 ` Tim Shen via gcc-patches
2017-06-01 15:13 ` Jonathan Wakely [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170601151302.GA10443@redhat.com \
--to=jwakely@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=libstdc++@gcc.gnu.org \
--cc=timshen@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).