This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.

Index Nav:	[Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav:	[Date Prev] [Date Next]	[Thread Prev] [Thread Next]
Other format:	[Raw text]

Re: [Patch] Forward triviality in variant

From: Jonathan Wakely <jwakely at redhat dot com>
To: Tim Shen <timshen at google dot com>
Cc: libstdc++ <libstdc++ at gcc dot gnu dot org>, gcc-patches <gcc-patches at gcc dot gnu dot org>
Date: Thu, 1 Jun 2017 16:13:02 +0100
Subject: Re: [Patch] Forward triviality in variant
Authentication-results: sourceware.org; auth=none
Authentication-results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
Authentication-results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=jwakely at redhat dot com
Dkim-filter: OpenDKIM Filter v2.11.0 mx1.redhat.com B1537804F7
Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com B1537804F7
References: <CAG4ZjNnp4DQXpBEs7yu-MCrfUGfk+3x=KkPZqJiQ5WWt3awnvg@mail.gmail.com> <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>>,

Follow-Ups:
- Re: [Patch] Forward triviality in variant
  - From: Ville Voutilainen
- Re: [Patch] Forward triviality in variant
  - From: Tim Shen via gcc-patches

Index Nav:	[Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav:	[Date Prev] [Date Next]	[Thread Prev] [Thread Next]