diff mbox

Forward triviality in variant

Message ID CAG4ZjNm1vWNAS8qytZaFV7DDdS3=fONkqjgUWUA4cY0ppsuaNg@mail.gmail.com
State New
Headers show

Commit Message

Li, Pan2 via Gcc-patches May 30, 2017, 9:16 a.m. UTC 
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.

Comments

Jonathan Wakely June 1, 2017, 3:13 p.m. UTC | #1 
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>>,
Ville Voutilainen June 1, 2017, 3:21 p.m. UTC | #2 
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.
Jonathan Wakely June 1, 2017, 3:29 p.m. UTC | #3 
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.
Ville Voutilainen June 1, 2017, 3:43 p.m. UTC | #4 
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.
Jonathan Wakely June 1, 2017, 4:03 p.m. UTC | #5 
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.
Ville Voutilainen June 1, 2017, 4:07 p.m. UTC | #6 
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.
Jonathan Wakely June 1, 2017, 4:13 p.m. UTC | #7 
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.
diff mbox

Patch

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>>, "");
+}