[v3,RFC] Rewrite variant. Also PR libstdc++/85517

Message ID CAFk2RUaH6GRp_FqyHXPso_jy2WVdrc_QtdvWz=+9w5F-mBW93A@mail.gmail.com
State New
Headers show
Series
  • [v3,RFC] Rewrite variant. Also PR libstdc++/85517
Related show

Commit Message

Ville Voutilainen Feb. 5, 2019, 10:05 p.m.
Okay then. This patch takes the hopefully biggest steps towards
a std::variant rewrite. The problem we have with the current
approach is that we'd really like to write fairly straightforward
code for variant's special member functions, but can't, because
the specification suggests fairly straightforward compile-time-property
queries and selection of behavior based on those, but variant's selected
index is a run-time property. This leads to difficulties implementing
what the standard requires, like the differences of handling throwing and
non-throwing move/copy operations of the types held in variant.

Well. We apply the hammer of Pattern Matching. variant's visit()
can get us into code that can do the compile-time queries despite
variant's selected index being a run-time property. We modify
the visitation mechanism to not throw an exception if invoked
with a special kind of visitor that can handle valueless variants,
and then use polylambdas as such visitors, and do if-constexprs
in those polylambdas.

We can now get rid of the function-pointer tables that are used for
similar kind of dispatching in the original approach. Visitation
generates similar tables, so we keep the O(1) indexing into the right
function based on the current selected index, and there's the same
amount of indirect calls through a pointer-to-function, one.
Our code for e.g. variant's copy assignment is now straightforward;
it visits both the this-variant and the rhs-variant at the same time,
and does the right thing based on the compile-time properties of the
selected type,
and whether either variant is valueless. Modulo the polylambda wrapping
and the fact that all ifs in it are if-constexprs, it looks like it's just
doing conditional code based on what the nothrow-properties, for example,
of the type of the object held by the variant are.

This patch also partially gets rid of some of the cases where an object
is self-destroyed before it's reconstructed. We shouldn't do that.
We should be doing _M_reset followed by _M_construct, so that we only
ever destroy a variant member of a union object held in the variant's
storage, and then try to reconstruct that (or some other variant member),
and we should never self-destroy any object in the inheritance chain of
variant. The rest of that is work-in-progress, as is changing the rest
of the special member functions to use visitation.

Thoughts?

2019-02-05  Ville Voutilainen  <ville.voutilainen@gmail.com>

    Rewrite variant.
    Also PR libstdc++/85517
    * include/std/variant (__do_visit): New.
    (__variant_cast): Likewise.
    (__variant_cookie): Likewise.
    (__erased_dtor): Remove.
    (_Variant_storage::_S_vtable): Likewise.
    (_Variant_storage::__M_reset_impl): Adjust to use __do_visit.
    (_Variant_storage::__M_reset): Adjust.
    (_Move_ctor_base::__M_destructive_copy): New.
    (_Copy_assign_base::operator=): Adjust to use __do_visit.
    (_Copy_assign_alias): Adjust to check both copy assignment
    and copy construction for triviality.
    (_Multi_array): Add support for visitors that accept and return
    a __variant_cookie.
    (__gen_vtable_impl::_S_apply_all_alts): Likewise.
    (__gen_vtable_impl::_S_apply_single_alt): Likewise.
    (__gen_vtable_impl::__element_by_index_or_cookie): New. Generate
    a __variant_cookie temporary for a variant that is valueless and..
    (__gen_vtable_impl::__visit_invoke): ..adjust here.
    (__gen_vtable::_Array_type): Conditionally make space for
    the __variant_cookie visitor case.
    (variant): Add __variant_cast as a friend.
    (variant::emplace): Use _M_reset() instead of self-destruction.
    (visit): Reimplement in terms of __do_visit.
    * testsuite/20_util/variant/compile.cc: Adjust.
    * testsuite/20_util/variant/run.cc: Likewise.

Comments

Jonathan Wakely Feb. 6, 2019, 10:13 a.m. | #1
On 06/02/19 00:05 +0200, Ville Voutilainen wrote:
>Okay then. This patch takes the hopefully biggest steps towards

>a std::variant rewrite. The problem we have with the current

>approach is that we'd really like to write fairly straightforward

>code for variant's special member functions, but can't, because

>the specification suggests fairly straightforward compile-time-property

>queries and selection of behavior based on those, but variant's selected

>index is a run-time property. This leads to difficulties implementing

>what the standard requires, like the differences of handling throwing and

>non-throwing move/copy operations of the types held in variant.

>

>Well. We apply the hammer of Pattern Matching. variant's visit()

>can get us into code that can do the compile-time queries despite

>variant's selected index being a run-time property. We modify

>the visitation mechanism to not throw an exception if invoked

>with a special kind of visitor that can handle valueless variants,

>and then use polylambdas as such visitors, and do if-constexprs

>in those polylambdas.

>

>We can now get rid of the function-pointer tables that are used for

>similar kind of dispatching in the original approach. Visitation

>generates similar tables, so we keep the O(1) indexing into the right

>function based on the current selected index, and there's the same

>amount of indirect calls through a pointer-to-function, one.

>Our code for e.g. variant's copy assignment is now straightforward;

>it visits both the this-variant and the rhs-variant at the same time,

>and does the right thing based on the compile-time properties of the

>selected type,

>and whether either variant is valueless. Modulo the polylambda wrapping

>and the fact that all ifs in it are if-constexprs, it looks like it's just

>doing conditional code based on what the nothrow-properties, for example,

>of the type of the object held by the variant are.


Did you compare the codegen?

I think the assumption was this would produce smaller code. Does that
hold true?

>This patch also partially gets rid of some of the cases where an object

>is self-destroyed before it's reconstructed. We shouldn't do that.


Agreed, that's been concerning me since I noticed it.

>We should be doing _M_reset followed by _M_construct, so that we only

>ever destroy a variant member of a union object held in the variant's

>storage, and then try to reconstruct that (or some other variant member),

>and we should never self-destroy any object in the inheritance chain of

>variant. The rest of that is work-in-progress, as is changing the rest

>of the special member functions to use visitation.

>

>Thoughts?


I like it. As you know, I've never liked that _S_vtable[] array of
function pointers, and the this->~variant() calls are undefined.

This is also necessary to implement the P0602R4 changes, and seems to
do so without ABI changes to our std::variant (its layout is not
changed by the patch).

As it only touches a C++17 component I'm inclined to approve it for
trunk before gcc-9.

Are you still working on the rest of the special member functions? It
seems to me that they could be fixed separately anyway, it doesn't
need to be done all at once. Each special member function is
independent of the others.
Ville Voutilainen Feb. 6, 2019, 10:21 a.m. | #2
On Wed, 6 Feb 2019 at 12:13, Jonathan Wakely <jwakely@redhat.com> wrote:
> Did you compare the codegen?


No. Getting the metaprograms to work took all the time I had thus far. :)

> I think the assumption was this would produce smaller code. Does that

> hold true?


I think the assumption was that this produces equivalent or better
code than fixing the current
semantics bugs with some sort of separate constexpr arrays of function
pointers would.
And certainly better code than any approach that has a run-time
branch. I don't think this ends up
being different from the current codegen; both have an array indexing
operation followed by an indirect
function call.

> Are you still working on the rest of the special member functions? It


Yes.

> seems to me that they could be fixed separately anyway, it doesn't

> need to be done all at once. Each special member function is

> independent of the others.


Right. The question is whether we want to commit a mixture of
visitation and separate function pointer
arrays. :)
Ville Voutilainen Feb. 6, 2019, 1:12 p.m. | #3
On Wed, 6 Feb 2019 at 12:21, Ville Voutilainen
<ville.voutilainen@gmail.com> wrote:
> > I think the assumption was this would produce smaller code. Does that

> > hold true?

>

> I think the assumption was that this produces equivalent or better

> code than fixing the current

> semantics bugs with some sort of separate constexpr arrays of function

> pointers would.

> And certainly better code than any approach that has a run-time

> branch. I don't think this ends up

> being different from the current codegen; both have an array indexing

> operation followed by an indirect

> function call.


And, to emphasize, the most important reason for this was to be able
to write straightforward
code for the special member functions, with the hope that it wouldn't
have a negative codegen
effect. Our Microsoft friends described the general technique as "has
crazy-good codegen",
but I have no idea what their starting point was; our starting point
probably wasn't bad
to begin with.
Ville Voutilainen March 3, 2019, 11:26 p.m. | #4
On Wed, 6 Feb 2019 at 15:12, Ville Voutilainen
<ville.voutilainen@gmail.com> wrote:

> And, to emphasize, the most important reason for this was to be able

> to write straightforward

> code for the special member functions, with the hope that it wouldn't

> have a negative codegen

> effect. Our Microsoft friends described the general technique as "has

> crazy-good codegen",

> but I have no idea what their starting point was; our starting point

> probably wasn't bad

> to begin with.


However, the codegen should be somewhat improved; this patch removes a
bag of run-time ifs from the implementation.

An amended patch attached. This gets rid of all __erased* stuff,
including hash, swap, constructors, relops.
I consider variant to no longer be in the state of sin after this.
Since this is touching just a C++17 facility with no
impact elsewhere, we could consider landing it in GCC 9 as a late
change. Failing that, it certainly seems safe enough
to put into GCC 9.2.

2019-03-04  Ville Voutilainen  <ville.voutilainen@gmail.com>

    Rewrite variant.
    Also PR libstdc++/85517
    * include/std/variant (__do_visit): New.
    (__variant_cast): Likewise.
    (__variant_cookie): Likewise.
    (__erased_*): Remove.
    (_Variant_storage::_S_vtable): Likewise.
    (_Variant_storage::__M_reset_impl): Adjust to use __do_visit.
    (_Variant_storage::__M_reset): Adjust.
    (_Copy_ctor_base(const _Copy_ctor_base&)): Adjust to use __do_visit.
    (_Move_ctor_base(_Move_ctor_base&&)): Likewise.
    (_Move_ctor_base::__M_destructive_copy): New.
    (_Copy_assign_base::operator=): Adjust to use __do_visit.
    (_Copy_assign_alias): Adjust to check both copy assignment
    and copy construction for triviality.
    (_Move_assign_base::operator=): Adjust to use __do_visit.
    (_Multi_array): Add support for visitors that accept and return
    a __variant_cookie.
    (__gen_vtable_impl::_S_apply_all_alts): Likewise.
    (__gen_vtable_impl::_S_apply_single_alt): Likewise.
    (__gen_vtable_impl::__element_by_index_or_cookie): New. Generate
    a __variant_cookie temporary for a variant that is valueless and..
    (__gen_vtable_impl::__visit_invoke): ..adjust here.
    (__gen_vtable::_Array_type): Conditionally make space for
    the __variant_cookie visitor case.
    (relops): Adjust to use __do_visit.
    (variant): Add __variant_cast as a friend.
    (variant::emplace): Use _M_reset() instead of self-destruction.
    (visit): Reimplement in terms of __do_visit.
    * testsuite/20_util/variant/compile.cc: Adjust.
    * testsuite/20_util/variant/run.cc: Likewise.
diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 89deb14..8b1f407 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -138,6 +138,19 @@ namespace __variant
     constexpr variant_alternative_t<_Np, variant<_Types...>> const&&
     get(const variant<_Types...>&&);
 
+  template<typename _Visitor, typename... _Variants>
+    constexpr decltype(auto)
+    __do_visit(_Visitor&& __visitor, _Variants&&... __variants);
+
+  template <typename... _Types, typename _Tp>
+    decltype(auto) __variant_cast(_Tp&& __rhs)
+    {
+      if constexpr (is_const_v<remove_reference_t<_Tp>>)
+        return static_cast<const variant<_Types...>&>(__rhs);
+      else
+        return static_cast<variant<_Types...>&>(__rhs);
+    }
+
 namespace __detail
 {
 namespace __variant
@@ -155,6 +168,9 @@ namespace __variant
       std::integral_constant<size_t, is_same_v<_Tp, _First>
 	? 0 : __index_of_v<_Tp, _Rest...> + 1> {};
 
+  // used for raw visitation
+  struct __variant_cookie {};
+
   // _Uninitialized<T> is guaranteed to be a literal type, even if T is not.
   // We have to do this, because [basic.types]p10.5.3 (n4606) is not implemented
   // yet. When it's implemented, _Uninitialized<T> can be changed to the alias
@@ -236,63 +252,6 @@ namespace __variant
 			      std::forward<_Variant>(__v)._M_u);
     }
 
-  // Various functions as "vtable" entries, where those vtables are used by
-  // polymorphic operations.
-  template<typename _Lhs, typename _Rhs>
-    void
-    __erased_ctor(void* __lhs, void* __rhs)
-    {
-      using _Type = remove_reference_t<_Lhs>;
-      ::new (__lhs) _Type(__variant::__ref_cast<_Rhs>(__rhs));
-    }
-
-  template<typename _Variant, size_t _Np>
-    void
-    __erased_dtor(_Variant&& __v)
-    { std::_Destroy(std::__addressof(__variant::__get<_Np>(__v))); }
-
-  template<typename _Lhs, typename _Rhs>
-    void
-    __erased_assign(void* __lhs, void* __rhs)
-    {
-      __variant::__ref_cast<_Lhs>(__lhs) = __variant::__ref_cast<_Rhs>(__rhs);
-    }
-
-  template<typename _Lhs, typename _Rhs>
-    void
-    __erased_swap(void* __lhs, void* __rhs)
-    {
-      using std::swap;
-      swap(__variant::__ref_cast<_Lhs>(__lhs),
-	   __variant::__ref_cast<_Rhs>(__rhs));
-    }
-
-#define _VARIANT_RELATION_FUNCTION_TEMPLATE(__OP, __NAME) \
-  template<typename _Variant, size_t _Np> \
-    constexpr bool \
-    __erased_##__NAME(const _Variant& __lhs, const _Variant& __rhs) \
-    { \
-      return __variant::__get<_Np>(std::forward<_Variant>(__lhs)) \
-	  __OP __variant::__get<_Np>(std::forward<_Variant>(__rhs)); \
-    }
-
-  _VARIANT_RELATION_FUNCTION_TEMPLATE(<, less)
-  _VARIANT_RELATION_FUNCTION_TEMPLATE(<=, less_equal)
-  _VARIANT_RELATION_FUNCTION_TEMPLATE(==, equal)
-  _VARIANT_RELATION_FUNCTION_TEMPLATE(!=, not_equal)
-  _VARIANT_RELATION_FUNCTION_TEMPLATE(>=, greater_equal)
-  _VARIANT_RELATION_FUNCTION_TEMPLATE(>, greater)
-
-#undef _VARIANT_RELATION_FUNCTION_TEMPLATE
-
-  template<typename _Tp>
-    size_t
-    __erased_hash(void* __t)
-    {
-      return std::hash<__remove_cvref_t<_Tp>>{}(
-	  __variant::__ref_cast<_Tp>(__t));
-    }
-
   template<typename... _Types>
     struct _Traits
     {
@@ -369,9 +328,6 @@ namespace __variant
   template<typename... _Types>
     struct _Variant_storage<false, _Types...>
     {
-      template<size_t... __indices>
-	static constexpr void (*_S_vtable[])(const _Variant_storage&) =
-	    { &__erased_dtor<const _Variant_storage&, __indices>... };
 
       constexpr _Variant_storage() : _M_index(variant_npos) { }
 
@@ -381,16 +337,21 @@ namespace __variant
 	_M_index(_Np)
 	{ }
 
-      template<size_t... __indices>
-	constexpr void _M_reset_impl(std::index_sequence<__indices...>)
-	{
-	  if (_M_index != __index_type(variant_npos))
-	    _S_vtable<__indices...>[_M_index](*this);
+      constexpr void _M_reset_impl()
+      {
+	__do_visit([](auto&& __this_mem) mutable
+		   -> __detail::__variant::__variant_cookie
+	  {
+	    if constexpr (!is_same_v<remove_reference_t<decltype(__this_mem)>,
+			  __variant_cookie>)
+	      std::_Destroy(std::__addressof(__this_mem));
+	    return {};
+	  }, __variant_cast<_Types...>(*this));
 	}
 
       void _M_reset()
       {
-	_M_reset_impl(std::index_sequence_for<_Types...>{});
+	_M_reset_impl();
 	_M_index = variant_npos;
       }
 
@@ -465,13 +426,18 @@ namespace __variant
       _Copy_ctor_base(const _Copy_ctor_base& __rhs)
 	  noexcept(_Traits<_Types...>::_S_nothrow_copy_ctor)
       {
-	if (__rhs._M_valid())
+	this->_M_index = __rhs._M_index;
+	__do_visit([](auto&& __this_mem, auto&& __rhs_mem) mutable
+		   -> __detail::__variant::__variant_cookie
 	  {
-	    static constexpr void (*_S_vtable[])(void*, void*) =
-	      { &__erased_ctor<_Types&, const _Types&>... };
-	    _S_vtable[__rhs._M_index](this->_M_storage(), __rhs._M_storage());
-	    this->_M_index = __rhs._M_index;
-	  }
+	    using _Type = remove_reference_t<decltype(__this_mem)>;
+	    if constexpr (is_same_v<__remove_cvref_t<decltype(__rhs_mem)>,
+			            remove_cv_t<_Type>>
+			  && !is_same_v<_Type, __variant_cookie>)
+	      ::new (&__this_mem) _Type(__rhs_mem);
+	    return {};
+	  }, __variant_cast<_Types...>(*this),
+	     __variant_cast<_Types...>(__rhs));
       }
 
       _Copy_ctor_base(_Copy_ctor_base&&) = default;
@@ -499,13 +465,18 @@ namespace __variant
       _Move_ctor_base(_Move_ctor_base&& __rhs)
 	  noexcept(_Traits<_Types...>::_S_nothrow_move_ctor)
       {
-	if (__rhs._M_valid())
+	this->_M_index = __rhs._M_index;
+	__do_visit([](auto&& __this_mem, auto&& __rhs_mem) mutable
+		   -> __detail::__variant::__variant_cookie
 	  {
-	    static constexpr void (*_S_vtable[])(void*, void*) =
-	      { &__erased_ctor<_Types&, _Types&&>... };
-	    _S_vtable[__rhs._M_index](this->_M_storage(), __rhs._M_storage());
-	    this->_M_index = __rhs._M_index;
-	  }
+	    using _Type = remove_reference_t<decltype(__this_mem)>;
+	    if constexpr (is_same_v<remove_reference_t<decltype(__rhs_mem)>,
+			            _Type>
+			  && !is_same_v<_Type, __variant_cookie>)
+	      ::new (&__this_mem) _Type(std::move(__rhs_mem));
+	    return {};
+	  }, __variant_cast<_Types...>(*this),
+	     __variant_cast<_Types...>(__rhs));
       }
 
       void _M_destructive_move(_Move_ctor_base&& __rhs)
@@ -522,6 +493,20 @@ namespace __variant
 	  }
       }
 
+      void _M_destructive_copy(const _Move_ctor_base& __rhs)
+      {
+	this->~_Move_ctor_base();
+	__try
+	  {
+	    ::new (this) _Move_ctor_base(__rhs);
+	  }
+	__catch (...)
+	  {
+	    this->_M_index = variant_npos;
+	    __throw_exception_again;
+	  }
+      }
+
       _Move_ctor_base(const _Move_ctor_base&) = default;
       _Move_ctor_base& operator=(const _Move_ctor_base&) = default;
       _Move_ctor_base& operator=(_Move_ctor_base&&) = default;
@@ -538,6 +523,11 @@ namespace __variant
 	this->~_Move_ctor_base();
 	::new (this) _Move_ctor_base(std::move(__rhs));
       }
+      void _M_destructive_copy(const _Move_ctor_base& __rhs)
+      {
+	this->~_Move_ctor_base();
+	::new (this) _Move_ctor_base(__rhs);
+      }
     };
 
   template<typename... _Types>
@@ -554,21 +544,44 @@ namespace __variant
       operator=(const _Copy_assign_base& __rhs)
 	  noexcept(_Traits<_Types...>::_S_nothrow_copy_assign)
       {
-	if (this->_M_index == __rhs._M_index)
+	__do_visit([this, &__rhs](auto&& __this_mem, auto&& __rhs_mem) mutable
+		   -> __detail::__variant::__variant_cookie
 	  {
-	    if (__rhs._M_valid())
+	    if constexpr (is_same_v<
+			    remove_reference_t<decltype(__this_mem)>,
+			    remove_reference_t<decltype(__rhs_mem)>>)
 	      {
-		static constexpr void (*_S_vtable[])(void*, void*) =
-		  { &__erased_assign<_Types&, const _Types&>... };
-		_S_vtable[__rhs._M_index](this->_M_storage(),
-					  __rhs._M_storage());
+		if constexpr (!is_same_v<
+			        remove_reference_t<decltype(__rhs_mem)>,
+			        __variant_cookie>)
+		  __this_mem = __rhs_mem;
 	      }
-	  }
-	else
-	  {
-	    _Copy_assign_base __tmp(__rhs);
-	    this->_M_destructive_move(std::move(__tmp));
-	  }
+	    else
+	      {
+		if constexpr (!is_same_v<
+			        remove_reference_t<decltype(__rhs_mem)>,
+			        __variant_cookie>)
+		  {
+		    using __rhs_type =
+		      remove_reference_t<decltype(__rhs_mem)>;
+		    if constexpr (is_nothrow_copy_constructible_v<__rhs_type>
+		      || !is_nothrow_move_constructible_v<__rhs_type>)
+		      {
+			this->_M_destructive_copy(__rhs);
+		      }
+		    else
+		      {
+			_Copy_assign_base __tmp(__rhs);
+			this->_M_destructive_move(std::move(__tmp));
+		      }
+		  }
+		else
+		  {
+		    this->_M_reset();
+		  }
+	      }
+	  return {};
+	}, __variant_cast<_Types...>(*this), __variant_cast<_Types...>(__rhs));
 	__glibcxx_assert(this->_M_index == __rhs._M_index);
 	return *this;
       }
@@ -587,7 +600,8 @@ namespace __variant
 
   template<typename... _Types>
     using _Copy_assign_alias =
-	_Copy_assign_base<_Traits<_Types...>::_S_trivial_copy_assign,
+	_Copy_assign_base<_Traits<_Types...>::_S_trivial_copy_assign
+			  && _Traits<_Types...>::_S_trivial_copy_ctor,
 			  _Types...>;
 
   template<bool, typename... _Types>
@@ -600,21 +614,25 @@ namespace __variant
       operator=(_Move_assign_base&& __rhs)
 	  noexcept(_Traits<_Types...>::_S_nothrow_move_assign)
       {
-	if (this->_M_index == __rhs._M_index)
+	__do_visit([this, &__rhs](auto&& __this_mem, auto&& __rhs_mem) mutable
+		   -> __detail::__variant::__variant_cookie
 	  {
-	    if (__rhs._M_valid())
+	    if constexpr (is_same_v<
+			    remove_reference_t<decltype(__this_mem)>,
+			    remove_reference_t<decltype(__rhs_mem)>>)
 	      {
-		static constexpr void (*_S_vtable[])(void*, void*) =
-		  { &__erased_assign<_Types&, _Types&&>... };
-		_S_vtable[__rhs._M_index]
-		  (this->_M_storage(), __rhs._M_storage());
+		if constexpr (!is_same_v<
+			        remove_reference_t<decltype(__rhs_mem)>,
+			        __variant_cookie>)
+		  __this_mem = std::move(__rhs_mem);
 	      }
-	  }
-	else
-	  {
-	    _Move_assign_base __tmp(std::move(__rhs));
-	    this->_M_destructive_move(std::move(__tmp));
-	  }
+	    else
+	      {
+		_Move_assign_base __tmp(std::move(__rhs));
+		this->_M_destructive_move(std::move(__tmp));
+	      }
+	  return {};
+	}, __variant_cast<_Types...>(*this), __variant_cast<_Types...>(__rhs));
 	__glibcxx_assert(this->_M_index == __rhs._M_index);
 	return *this;
       }
@@ -733,15 +751,21 @@ namespace __variant
       _Tp _M_data;
     };
 
-  template<typename _Tp, size_t __first, size_t... __rest>
-    struct _Multi_array<_Tp, __first, __rest...>
+  template<typename _Ret,
+	   typename _Visitor,
+	   typename... _Variants,
+	   size_t __first, size_t... __rest>
+    struct _Multi_array<_Ret(*)(_Visitor, _Variants...), __first, __rest...>
     {
+      static constexpr int __do_cookie =
+	is_same_v<_Ret, __variant_cookie> ? 1 : 0;
+      using _Tp = _Ret(*)(_Visitor, _Variants...);
       template<typename... _Args>
 	constexpr const _Tp&
 	_M_access(size_t __first_index, _Args... __rest_indices) const
-	{ return _M_arr[__first_index]._M_access(__rest_indices...); }
+        { return _M_arr[__first_index + __do_cookie]._M_access(__rest_indices...); }
 
-      _Multi_array<_Tp, __rest...> _M_arr[__first];
+      _Multi_array<_Tp, __rest...> _M_arr[__first + __do_cookie];
     };
 
   // Creates a multi-dimensional vtable recursively.
@@ -801,18 +825,37 @@ namespace __variant
 	_S_apply_all_alts(_Array_type& __vtable,
 			  std::index_sequence<__var_indices...>)
 	{
-	  (_S_apply_single_alt<__var_indices>(
-	     __vtable._M_arr[__var_indices]), ...);
+	  if constexpr (is_same_v<_Result_type, __variant_cookie>)
+	    (_S_apply_single_alt<true, __var_indices>(
+	      __vtable._M_arr[__var_indices + 1],
+	      &(__vtable._M_arr[0])), ...);
+	  else
+	    (_S_apply_single_alt<false, __var_indices>(
+	      __vtable._M_arr[__var_indices]), ...);
 	}
 
-      template<size_t __index, typename _Tp>
+      template<bool __do_cookie, size_t __index, typename _Tp>
 	static constexpr void
-	_S_apply_single_alt(_Tp& __element)
+	_S_apply_single_alt(_Tp& __element, _Tp* __cookie_element = nullptr)
 	{
 	  using _Alternative = variant_alternative_t<__index, _Next>;
-	  __element = __gen_vtable_impl<
-	    remove_reference_t<decltype(__element)>, tuple<_Variants...>,
-	    std::index_sequence<__indices..., __index>>::_S_apply();
+	  if constexpr (__do_cookie)
+	    {
+	      __element = __gen_vtable_impl<
+		_Tp,
+		tuple<_Variants...>,
+		std::index_sequence<__indices..., __index>>::_S_apply();
+	      *__cookie_element = __gen_vtable_impl<
+		_Tp,
+		tuple<_Variants...>,
+		std::index_sequence<__indices..., variant_npos>>::_S_apply();
+	    }
+	  else
+	    {
+	      __element = __gen_vtable_impl<
+		remove_reference_t<decltype(__element)>, tuple<_Variants...>,
+		std::index_sequence<__indices..., __index>>::_S_apply();
+	    }
 	}
     };
 
@@ -825,11 +868,22 @@ namespace __variant
       using _Array_type =
 	  _Multi_array<_Result_type (*)(_Visitor&&, _Variants...)>;
 
+      template<size_t __index, typename _Variant>
+	static constexpr decltype(auto)
+	__element_by_index_or_cookie(_Variant&& __var)
+        {
+	  if constexpr (__index != variant_npos)
+	    return __variant::__get<__index>(std::forward<_Variant>(__var));
+	  else
+	    return __variant_cookie{};
+	}
+
       static constexpr decltype(auto)
       __visit_invoke(_Visitor&& __visitor, _Variants... __vars)
       {
 	return std::__invoke(std::forward<_Visitor>(__visitor),
-	    __variant::__get<__indices>(std::forward<_Variants>(__vars))...);
+	    __element_by_index_or_cookie<__indices>(
+	      std::forward<_Variants>(__vars))...);
       }
 
       static constexpr auto
@@ -843,7 +897,9 @@ namespace __variant
       using _Func_ptr = _Result_type (*)(_Visitor&&, _Variants...);
       using _Array_type =
 	  _Multi_array<_Func_ptr,
-		       variant_size_v<remove_reference_t<_Variants>>...>;
+		       (variant_size_v<remove_reference_t<_Variants>>
+			+ (is_same_v<_Result_type, __variant_cookie> ? 1 : 0))
+	               ...>;
 
       static constexpr _Array_type
       _S_apply()
@@ -973,7 +1029,27 @@ namespace __variant
     constexpr bool operator __OP(const variant<_Types...>& __lhs, \
 				 const variant<_Types...>& __rhs) \
     { \
-      return __lhs._M_##__NAME(__rhs, std::index_sequence_for<_Types...>{}); \
+      bool __ret = true; \
+      __do_visit([&__ret, &__lhs, __rhs] \
+		 (auto&& __this_mem, auto&& __rhs_mem) mutable	\
+		   -> __detail::__variant::__variant_cookie \
+        { \
+	  if constexpr (!is_same_v< \
+			  remove_reference_t<decltype(__this_mem)>, \
+			  remove_reference_t<decltype(__rhs_mem)>> \
+			|| is_same_v<decltype(__this_mem), \
+			             __detail::__variant::__variant_cookie>) \
+	    __ret = (__lhs.index() + 1) __OP (__rhs.index() + 1); \
+	  else if constexpr (is_same_v< \
+			       remove_reference_t<decltype(__this_mem)>, \
+			       remove_reference_t<decltype(__rhs_mem)>> \
+                             && !is_same_v< \
+	                          remove_reference_t<decltype(__this_mem)>, \
+				  __detail::__variant::__variant_cookie>) \
+	    __ret = __this_mem __OP __rhs_mem; \
+	  return {}; \
+	}, __lhs, __rhs); \
+      return __ret; \
     } \
 \
   constexpr bool operator __OP(monostate, monostate) noexcept \
@@ -1036,6 +1112,9 @@ namespace __variant
 	variant<_Types...>>
     {
     private:
+      template <typename... _UTypes, typename _Tp>
+	friend decltype(auto) __variant_cast(_Tp&&);
+
       static_assert(sizeof...(_Types) > 0,
 		    "variant must have at least one alternative");
       static_assert(!(std::is_reference_v<_Types> || ...),
@@ -1185,7 +1264,6 @@ namespace __variant
 	{
 	  static_assert(_Np < sizeof...(_Types),
 			"The index should be in [0, number of alternatives)");
-
 	  using type = variant_alternative_t<_Np, variant>;
 	  // If constructing the value can throw but move assigning it can't,
 	  // construct in a temporary and then move assign from it. This gives
@@ -1202,7 +1280,7 @@ namespace __variant
 	      return std::get<_Np>(*this);
 	    }
 
-	  this->~variant();
+	  this->_M_reset();
 	  __try
 	    {
 	      ::new (this) variant(in_place_index<_Np>,
@@ -1225,7 +1303,6 @@ namespace __variant
 	{
 	  static_assert(_Np < sizeof...(_Types),
 			"The index should be in [0, number of alternatives)");
-
 	  using type = variant_alternative_t<_Np, variant>;
 	  if constexpr (is_trivially_copyable_v<type>
 	      && !is_nothrow_constructible_v<type, initializer_list<_Up>,
@@ -1239,7 +1316,7 @@ namespace __variant
 	      return std::get<_Np>(*this);
 	    }
 
-	  this->~variant();
+	  this->_M_reset();
 	  __try
 	    {
 	      ::new (this) variant(in_place_index<_Np>, __il,
@@ -1270,62 +1347,49 @@ namespace __variant
       noexcept((__is_nothrow_swappable<_Types>::value && ...)
 	       && is_nothrow_move_constructible_v<variant>)
       {
-	if (this->index() == __rhs.index())
+	__do_visit([this, &__rhs](auto&& __this_mem, auto&& __rhs_mem) mutable
+		   -> __detail::__variant::__variant_cookie
 	  {
-	    if (this->_M_valid())
+	    if constexpr (is_same_v<
+			    remove_reference_t<decltype(__this_mem)>,
+			    remove_reference_t<decltype(__rhs_mem)>>)
 	      {
-		static constexpr void (*_S_vtable[])(void*, void*) =
-		  { &__detail::__variant::__erased_swap<_Types&, _Types&>... };
-		_S_vtable[__rhs._M_index](this->_M_storage(),
-					  __rhs._M_storage());
+		if constexpr (!is_same_v<
+			        remove_reference_t<decltype(__rhs_mem)>,
+			        __detail::__variant::__variant_cookie>)
+		  {
+		    using std::swap;
+		    swap(__this_mem, __rhs_mem);
+		  }
 	      }
-	  }
-	else if (!this->_M_valid())
-	  {
-	    this->_M_destructive_move(std::move(__rhs));
-	    __rhs._M_reset();
-	  }
-	else if (!__rhs._M_valid())
-	  {
-	    __rhs._M_destructive_move(std::move(*this));
-	    this->_M_reset();
-	  }
-	else
-	  {
-	    auto __tmp = std::move(__rhs);
-	    __rhs._M_destructive_move(std::move(*this));
-	    this->_M_destructive_move(std::move(__tmp));
-	  }
+	    else
+	      {
+		if constexpr (is_same_v<
+			        remove_reference_t<decltype(__this_mem)>,
+			        __detail::__variant::__variant_cookie>)
+		  {
+		    this->_M_destructive_move(std::move(__rhs));
+		    __rhs._M_reset();
+		  }
+		else if constexpr (is_same_v<
+			             remove_reference_t<decltype(__rhs_mem)>,
+			             __detail::__variant::__variant_cookie>)
+		  {
+		    __rhs._M_destructive_move(std::move(*this));
+		    this->_M_reset();
+		  }
+		else
+		  {
+		    auto __tmp = std::move(__rhs);
+		    __rhs._M_destructive_move(std::move(*this));
+		    this->_M_destructive_move(std::move(__tmp));
+		  }
+	      }
+	  return {};
+	}, *this, __rhs);
       }
 
     private:
-#define _VARIANT_RELATION_FUNCTION_TEMPLATE(__OP, __NAME) \
-      template<size_t... __indices> \
-	static constexpr bool \
-	(*_S_erased_##__NAME[])(const variant&, const variant&) = \
-	  { &__detail::__variant::__erased_##__NAME< \
-		const variant&, __indices>... }; \
-      template<size_t... __indices> \
-	constexpr bool \
-	_M_##__NAME(const variant& __rhs, \
-		    std::index_sequence<__indices...>) const \
-	{ \
-	  auto __lhs_index = this->index(); \
-	  auto __rhs_index = __rhs.index(); \
-	  if (__lhs_index != __rhs_index || valueless_by_exception()) \
-	    /* Modulo addition. */ \
-	    return __lhs_index + 1 __OP __rhs_index + 1; \
-	  return _S_erased_##__NAME<__indices...>[__lhs_index](*this, __rhs); \
-	}
-
-      _VARIANT_RELATION_FUNCTION_TEMPLATE(<, less)
-      _VARIANT_RELATION_FUNCTION_TEMPLATE(<=, less_equal)
-      _VARIANT_RELATION_FUNCTION_TEMPLATE(==, equal)
-      _VARIANT_RELATION_FUNCTION_TEMPLATE(!=, not_equal)
-      _VARIANT_RELATION_FUNCTION_TEMPLATE(>=, greater_equal)
-      _VARIANT_RELATION_FUNCTION_TEMPLATE(>, greater)
-
-#undef _VARIANT_RELATION_FUNCTION_TEMPLATE
 
 #if defined(__clang__) && __clang_major__ <= 7
     public:
@@ -1401,11 +1465,8 @@ namespace __variant
 
   template<typename _Visitor, typename... _Variants>
     constexpr decltype(auto)
-    visit(_Visitor&& __visitor, _Variants&&... __variants)
+    __do_visit(_Visitor&& __visitor, _Variants&&... __variants)
     {
-      if ((__variants.valueless_by_exception() || ...))
-	__throw_bad_variant_access("Unexpected index");
-
       using _Result_type =
 	decltype(std::forward<_Visitor>(__visitor)(
 	    std::get<0>(std::forward<_Variants>(__variants))...));
@@ -1418,6 +1479,17 @@ namespace __variant
 			   std::forward<_Variants>(__variants)...);
     }
 
+  template<typename _Visitor, typename... _Variants>
+    constexpr decltype(auto)
+    visit(_Visitor&& __visitor, _Variants&&... __variants)
+    {
+      if ((__variants.valueless_by_exception() || ...))
+	__throw_bad_variant_access("Unexpected index");
+
+      return __do_visit(std::forward<_Visitor>(__visitor),
+			std::forward<_Variants>(__variants)...);
+    }
+
   template<bool, typename... _Types>
     struct __variant_hash_call_base_impl
     {
@@ -1425,15 +1497,20 @@ namespace __variant
       operator()(const variant<_Types...>& __t) const
       noexcept((is_nothrow_invocable_v<hash<decay_t<_Types>>, _Types> && ...))
       {
-	if (!__t.valueless_by_exception())
+	size_t __ret;
+	__do_visit([&__t, &__ret](auto&& __t_mem) mutable
+		   -> __detail::__variant::__variant_cookie
 	  {
-	    namespace __edv = __detail::__variant;
-	    static constexpr size_t (*_S_vtable[])(void*) =
-	      { &__edv::__erased_hash<const _Types&>... };
-	    return hash<size_t>{}(__t.index())
-	      + _S_vtable[__t.index()](__edv::__get_storage(__t));
-	  }
-	return hash<size_t>{}(__t.index());
+	    using _Type = __remove_cvref_t<decltype(__t_mem)>;
+	    if constexpr (!is_same_v<_Type,
+			             __detail::__variant::__variant_cookie>)
+	      __ret = std::hash<size_t>{}(__t.index())
+		      + std::hash<_Type>{}(__t_mem);
+	    else
+	      __ret = std::hash<size_t>{}(__t.index());
+	    return {};
+	  }, __t);
+	return __ret;
       }
     };
 
diff --git a/libstdc++-v3/testsuite/20_util/variant/compile.cc b/libstdc++-v3/testsuite/20_util/variant/compile.cc
index 8a43092..04fef0b 100644
--- a/libstdc++-v3/testsuite/20_util/variant/compile.cc
+++ b/libstdc++-v3/testsuite/20_util/variant/compile.cc
@@ -488,12 +488,12 @@ void test_triviality()
   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, =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,  true,  true)
-  TEST_TEMPLATE(=default,         ,         , =default,         , false, false,  true, false)
+  TEST_TEMPLATE(=default,         ,         , =default, =default, false, false,  false,  true)
+  TEST_TEMPLATE(=default,         ,         , =default,         , false, false,  false, 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)
diff --git a/libstdc++-v3/testsuite/20_util/variant/run.cc b/libstdc++-v3/testsuite/20_util/variant/run.cc
index c5ea7df..7ee9b08 100644
--- a/libstdc++-v3/testsuite/20_util/variant/run.cc
+++ b/libstdc++-v3/testsuite/20_util/variant/run.cc
@@ -88,6 +88,21 @@ void arbitrary_ctor()
   VERIFY(get<1>(v) == "a");
 }
 
+struct ThrowingMoveCtorThrowsCopyCtor
+{
+  ThrowingMoveCtorThrowsCopyCtor() noexcept = default;
+  ThrowingMoveCtorThrowsCopyCtor(ThrowingMoveCtorThrowsCopyCtor&&) {}
+  ThrowingMoveCtorThrowsCopyCtor(ThrowingMoveCtorThrowsCopyCtor const&)
+  {
+    throw 0;
+  }
+
+  ThrowingMoveCtorThrowsCopyCtor& operator=(ThrowingMoveCtorThrowsCopyCtor&&) noexcept
+    = default;
+  ThrowingMoveCtorThrowsCopyCtor& operator=(ThrowingMoveCtorThrowsCopyCtor const&) noexcept
+    = default;
+};
+
 void copy_assign()
 {
   variant<monostate, string> v("a");
@@ -96,6 +111,20 @@ void copy_assign()
   u = v;
   VERIFY(holds_alternative<string>(u));
   VERIFY(get<string>(u) == "a");
+  {
+    std::variant<int, ThrowingMoveCtorThrowsCopyCtor> v1,
+      v2 = ThrowingMoveCtorThrowsCopyCtor();
+    bool should_throw = false;
+    try
+      {
+	v1 = v2;
+      }
+    catch(int)
+      {
+	should_throw = true;
+      }
+    VERIFY(should_throw);
+  }
 }
 
 void move_assign()
@@ -183,11 +212,15 @@ void emplace()
     AlwaysThrow a;
     try { v.emplace<1>(a); } catch (nullptr_t) { }
     VERIFY(v.valueless_by_exception());
+    v.emplace<0>(42);
+    VERIFY(!v.valueless_by_exception());
   }
   {
     variant<int, AlwaysThrow> v;
     try { v.emplace<1>(AlwaysThrow{}); } catch (nullptr_t) { }
     VERIFY(v.valueless_by_exception());
+    v.emplace<0>(42);
+    VERIFY(!v.valueless_by_exception());
   }
   VERIFY(&v.emplace<0>(1) == &std::get<0>(v));
   VERIFY(&v.emplace<int>(1) == &std::get<int>(v));
@@ -258,6 +291,7 @@ void test_relational()
     VERIFY(v < w);
     VERIFY(v <= w);
     VERIFY(!(v == w));
+    VERIFY(v == v);
     VERIFY(v != w);
     VERIFY(w > v);
     VERIFY(w >= v);
Ville Voutilainen March 3, 2019, 11:43 p.m. | #5
On Mon, 4 Mar 2019 at 01:26, Ville Voutilainen
<ville.voutilainen@gmail.com> wrote:
> I consider variant to no longer be in the state of sin after this.


Sigh, except for the places where it self-destructs or placement-news
over things that it shouldn't. That's hopefully
the next bit that I'll rectify, Real Soon Now.
Jonathan Wakely March 5, 2019, 4:22 p.m. | #6
On 04/03/19 01:26 +0200, Ville Voutilainen wrote:
>On Wed, 6 Feb 2019 at 15:12, Ville Voutilainen

><ville.voutilainen@gmail.com> wrote:

>

>> And, to emphasize, the most important reason for this was to be able

>> to write straightforward

>> code for the special member functions, with the hope that it wouldn't

>> have a negative codegen

>> effect. Our Microsoft friends described the general technique as "has

>> crazy-good codegen",

>> but I have no idea what their starting point was; our starting point

>> probably wasn't bad

>> to begin with.

>

>However, the codegen should be somewhat improved; this patch removes a

>bag of run-time ifs from the implementation.

>

>An amended patch attached. This gets rid of all __erased* stuff,

>including hash, swap, constructors, relops.

>I consider variant to no longer be in the state of sin after this.

>Since this is touching just a C++17 facility with no

>impact elsewhere, we could consider landing it in GCC 9 as a late

>change. Failing that, it certainly seems safe enough

>to put into GCC 9.2.


As it doesn't touch anything pre-C++17 it's OK for trunk now.

Thanks.
Ville Voutilainen March 5, 2019, 9:27 p.m. | #7
On Mon, 4 Mar 2019 at 01:43, Ville Voutilainen
<ville.voutilainen@gmail.com> wrote:
>

> On Mon, 4 Mar 2019 at 01:26, Ville Voutilainen

> <ville.voutilainen@gmail.com> wrote:

> > I consider variant to no longer be in the state of sin after this.

>

> Sigh, except for the places where it self-destructs or placement-news

> over things that it shouldn't. That's hopefully

> the next bit that I'll rectify, Real Soon Now.


Here we go. Variant, go forth and sin no more. Tested locally on
linux-x64, will run the full suite
on linux-ppc64. OK for trunk if tests pass?

2019-03-05  Ville Voutilainen  <ville.voutilainen@gmail.com>

    Rewrite variant.
    Also PR libstdc++/85517
    * include/std/variant (__do_visit): New.
    (__variant_cast): Likewise.
    (__variant_cookie): Likewise.
    (__erased_*): Remove.
    (_Variant_storage::_S_vtable): Likewise.
    (_Variant_storage::__M_reset_impl): Adjust to use __do_visit.
    (_Variant_storage::__M_reset): Adjust.
    (__variant_construct): New.
    (_Copy_ctor_base(const _Copy_ctor_base&)): Adjust to use
    __variant_construct.
    (_Move_ctor_base(_Move_ctor_base&&)): Likewise.
    (_Move_ctor_base::__M_destructive_copy): New.
    (_Move_ctor_base::__M_destructive_move): Adjust to use
    __variant_construct.
    (_Copy_assign_base::operator=): Adjust to use __do_visit.
    (_Copy_assign_alias): Adjust to check both copy assignment
    and copy construction for triviality.
    (_Move_assign_base::operator=): Adjust to use __do_visit.
    (_Multi_array): Add support for visitors that accept and return
    a __variant_cookie.
    (__gen_vtable_impl::_S_apply_all_alts): Likewise.
    (__gen_vtable_impl::_S_apply_single_alt): Likewise.
    (__gen_vtable_impl::__element_by_index_or_cookie): New. Generate
    a __variant_cookie temporary for a variant that is valueless and..
    (__gen_vtable_impl::__visit_invoke): ..adjust here.
    (__gen_vtable::_Array_type): Conditionally make space for
    the __variant_cookie visitor case.
    (__variant_construct_by_index): New.
    (get_if): Adjust to use std::addressof.
    (relops): Adjust to use __do_visit.
    (variant): Add __variant_cast and __variant_construct_by_index
    as friends.
    (variant::emplace): Use _M_reset() and __variant_construct_by_index
    instead of self-destruction.
    (variant::swap): Adjust to use __do_visit.
    (visit): Reimplement in terms of __do_visit.
    (__variant_hash_call_base_impl::operator()): Adjust to use __do_visit.
    * testsuite/20_util/variant/compile.cc: Adjust.
    * testsuite/20_util/variant/run.cc: Likewise.
diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 89deb14..5138599 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -138,6 +138,21 @@ namespace __variant
     constexpr variant_alternative_t<_Np, variant<_Types...>> const&&
     get(const variant<_Types...>&&);
 
+  template<typename _Visitor, typename... _Variants>
+    constexpr decltype(auto)
+    __do_visit(_Visitor&& __visitor, _Variants&&... __variants);
+
+  template <typename... _Types, typename _Tp>
+    decltype(auto) __variant_cast(_Tp&& __rhs)
+    {
+      if constexpr (is_const_v<remove_reference_t<_Tp>>)
+        return static_cast<const variant<_Types...>&>(__rhs);
+      else if constexpr (is_rvalue_reference_v<_Tp&&>)
+        return static_cast<variant<_Types...>&&>(__rhs);
+      else
+        return static_cast<variant<_Types...>&>(__rhs);
+    }
+
 namespace __detail
 {
 namespace __variant
@@ -155,6 +170,9 @@ namespace __variant
       std::integral_constant<size_t, is_same_v<_Tp, _First>
 	? 0 : __index_of_v<_Tp, _Rest...> + 1> {};
 
+  // used for raw visitation
+  struct __variant_cookie {};
+
   // _Uninitialized<T> is guaranteed to be a literal type, even if T is not.
   // We have to do this, because [basic.types]p10.5.3 (n4606) is not implemented
   // yet. When it's implemented, _Uninitialized<T> can be changed to the alias
@@ -236,63 +254,6 @@ namespace __variant
 			      std::forward<_Variant>(__v)._M_u);
     }
 
-  // Various functions as "vtable" entries, where those vtables are used by
-  // polymorphic operations.
-  template<typename _Lhs, typename _Rhs>
-    void
-    __erased_ctor(void* __lhs, void* __rhs)
-    {
-      using _Type = remove_reference_t<_Lhs>;
-      ::new (__lhs) _Type(__variant::__ref_cast<_Rhs>(__rhs));
-    }
-
-  template<typename _Variant, size_t _Np>
-    void
-    __erased_dtor(_Variant&& __v)
-    { std::_Destroy(std::__addressof(__variant::__get<_Np>(__v))); }
-
-  template<typename _Lhs, typename _Rhs>
-    void
-    __erased_assign(void* __lhs, void* __rhs)
-    {
-      __variant::__ref_cast<_Lhs>(__lhs) = __variant::__ref_cast<_Rhs>(__rhs);
-    }
-
-  template<typename _Lhs, typename _Rhs>
-    void
-    __erased_swap(void* __lhs, void* __rhs)
-    {
-      using std::swap;
-      swap(__variant::__ref_cast<_Lhs>(__lhs),
-	   __variant::__ref_cast<_Rhs>(__rhs));
-    }
-
-#define _VARIANT_RELATION_FUNCTION_TEMPLATE(__OP, __NAME) \
-  template<typename _Variant, size_t _Np> \
-    constexpr bool \
-    __erased_##__NAME(const _Variant& __lhs, const _Variant& __rhs) \
-    { \
-      return __variant::__get<_Np>(std::forward<_Variant>(__lhs)) \
-	  __OP __variant::__get<_Np>(std::forward<_Variant>(__rhs)); \
-    }
-
-  _VARIANT_RELATION_FUNCTION_TEMPLATE(<, less)
-  _VARIANT_RELATION_FUNCTION_TEMPLATE(<=, less_equal)
-  _VARIANT_RELATION_FUNCTION_TEMPLATE(==, equal)
-  _VARIANT_RELATION_FUNCTION_TEMPLATE(!=, not_equal)
-  _VARIANT_RELATION_FUNCTION_TEMPLATE(>=, greater_equal)
-  _VARIANT_RELATION_FUNCTION_TEMPLATE(>, greater)
-
-#undef _VARIANT_RELATION_FUNCTION_TEMPLATE
-
-  template<typename _Tp>
-    size_t
-    __erased_hash(void* __t)
-    {
-      return std::hash<__remove_cvref_t<_Tp>>{}(
-	  __variant::__ref_cast<_Tp>(__t));
-    }
-
   template<typename... _Types>
     struct _Traits
     {
@@ -369,9 +330,6 @@ namespace __variant
   template<typename... _Types>
     struct _Variant_storage<false, _Types...>
     {
-      template<size_t... __indices>
-	static constexpr void (*_S_vtable[])(const _Variant_storage&) =
-	    { &__erased_dtor<const _Variant_storage&, __indices>... };
 
       constexpr _Variant_storage() : _M_index(variant_npos) { }
 
@@ -381,16 +339,21 @@ namespace __variant
 	_M_index(_Np)
 	{ }
 
-      template<size_t... __indices>
-	constexpr void _M_reset_impl(std::index_sequence<__indices...>)
-	{
-	  if (_M_index != __index_type(variant_npos))
-	    _S_vtable<__indices...>[_M_index](*this);
+      constexpr void _M_reset_impl()
+      {
+	__do_visit([](auto&& __this_mem) mutable
+		   -> __detail::__variant::__variant_cookie
+	  {
+	    if constexpr (!is_same_v<remove_reference_t<decltype(__this_mem)>,
+			  __variant_cookie>)
+	      std::_Destroy(std::__addressof(__this_mem));
+	    return {};
+	  }, __variant_cast<_Types...>(*this));
 	}
 
       void _M_reset()
       {
-	_M_reset_impl(std::index_sequence_for<_Types...>{});
+	_M_reset_impl();
 	_M_index = variant_npos;
       }
 
@@ -453,6 +416,24 @@ namespace __variant
     using _Variant_storage_alias =
 	_Variant_storage<_Traits<_Types...>::_S_trivial_dtor, _Types...>;
 
+  template<typename... _Types, typename _Tp, typename _Up>
+    void __variant_construct(_Tp&& __lhs, _Up&& __rhs)
+    {
+      __lhs._M_index = __rhs._M_index;
+      __do_visit([](auto&& __this_mem, auto&& __rhs_mem) mutable
+		 -> __detail::__variant::__variant_cookie
+        {
+	  using _Type = remove_reference_t<decltype(__this_mem)>;
+	  if constexpr (is_same_v<__remove_cvref_t<decltype(__rhs_mem)>,
+			          remove_cv_t<_Type>>
+			&& !is_same_v<_Type, __variant_cookie>)
+	    ::new (std::addressof(__this_mem))
+	      _Type(std::forward<decltype(__rhs_mem)>(__rhs_mem));
+	  return {};
+	}, __variant_cast<_Types...>(__lhs),
+	   __variant_cast<_Types...>(std::forward<decltype(__rhs)>(__rhs)));
+    }
+
   // The following are (Copy|Move) (ctor|assign) layers for forwarding
   // triviality and handling non-trivial SMF behaviors.
 
@@ -465,13 +446,7 @@ namespace __variant
       _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](this->_M_storage(), __rhs._M_storage());
-	    this->_M_index = __rhs._M_index;
-	  }
+	__variant_construct<_Types...>(*this, __rhs);
       }
 
       _Copy_ctor_base(_Copy_ctor_base&&) = default;
@@ -499,21 +474,31 @@ namespace __variant
       _Move_ctor_base(_Move_ctor_base&& __rhs)
 	  noexcept(_Traits<_Types...>::_S_nothrow_move_ctor)
       {
-	if (__rhs._M_valid())
+	__variant_construct<_Types...>(*this,
+	  std::forward<_Move_ctor_base>(__rhs));
+      }
+
+      void _M_destructive_move(_Move_ctor_base&& __rhs)
+      {
+	this->_M_reset();
+	__try
 	  {
-	    static constexpr void (*_S_vtable[])(void*, void*) =
-	      { &__erased_ctor<_Types&, _Types&&>... };
-	    _S_vtable[__rhs._M_index](this->_M_storage(), __rhs._M_storage());
-	    this->_M_index = __rhs._M_index;
+	    __variant_construct<_Types...>(*this,
+	      std::forward<_Move_ctor_base>(__rhs));
+	  }
+	__catch (...)
+	  {
+	    this->_M_index = variant_npos;
+	    __throw_exception_again;
 	  }
       }
 
-      void _M_destructive_move(_Move_ctor_base&& __rhs)
+      void _M_destructive_copy(const _Move_ctor_base& __rhs)
       {
-	this->~_Move_ctor_base();
+	this->_M_reset();
 	__try
 	  {
-	    ::new (this) _Move_ctor_base(std::move(__rhs));
+	    __variant_construct<_Types...>(*this, __rhs);
 	  }
 	__catch (...)
 	  {
@@ -535,8 +520,14 @@ namespace __variant
 
       void _M_destructive_move(_Move_ctor_base&& __rhs)
       {
-	this->~_Move_ctor_base();
-	::new (this) _Move_ctor_base(std::move(__rhs));
+	this->_M_reset();
+	__variant_construct<_Types...>(*this,
+          std::forward<_Move_ctor_base>(__rhs));
+      }
+      void _M_destructive_copy(const _Move_ctor_base& __rhs)
+      {
+	this->_M_reset();
+	__variant_construct<_Types...>(*this, __rhs);
       }
     };
 
@@ -554,21 +545,44 @@ namespace __variant
       operator=(const _Copy_assign_base& __rhs)
 	  noexcept(_Traits<_Types...>::_S_nothrow_copy_assign)
       {
-	if (this->_M_index == __rhs._M_index)
+	__do_visit([this, &__rhs](auto&& __this_mem, auto&& __rhs_mem) mutable
+		   -> __detail::__variant::__variant_cookie
 	  {
-	    if (__rhs._M_valid())
+	    if constexpr (is_same_v<
+			    remove_reference_t<decltype(__this_mem)>,
+			    remove_reference_t<decltype(__rhs_mem)>>)
 	      {
-		static constexpr void (*_S_vtable[])(void*, void*) =
-		  { &__erased_assign<_Types&, const _Types&>... };
-		_S_vtable[__rhs._M_index](this->_M_storage(),
-					  __rhs._M_storage());
+		if constexpr (!is_same_v<
+			        remove_reference_t<decltype(__rhs_mem)>,
+			        __variant_cookie>)
+		  __this_mem = __rhs_mem;
 	      }
-	  }
-	else
-	  {
-	    _Copy_assign_base __tmp(__rhs);
-	    this->_M_destructive_move(std::move(__tmp));
-	  }
+	    else
+	      {
+		if constexpr (!is_same_v<
+			        remove_reference_t<decltype(__rhs_mem)>,
+			        __variant_cookie>)
+		  {
+		    using __rhs_type =
+		      remove_reference_t<decltype(__rhs_mem)>;
+		    if constexpr (is_nothrow_copy_constructible_v<__rhs_type>
+		      || !is_nothrow_move_constructible_v<__rhs_type>)
+		      {
+			this->_M_destructive_copy(__rhs);
+		      }
+		    else
+		      {
+			_Copy_assign_base __tmp(__rhs);
+			this->_M_destructive_move(std::move(__tmp));
+		      }
+		  }
+		else
+		  {
+		    this->_M_reset();
+		  }
+	      }
+	  return {};
+	}, __variant_cast<_Types...>(*this), __variant_cast<_Types...>(__rhs));
 	__glibcxx_assert(this->_M_index == __rhs._M_index);
 	return *this;
       }
@@ -587,7 +601,8 @@ namespace __variant
 
   template<typename... _Types>
     using _Copy_assign_alias =
-	_Copy_assign_base<_Traits<_Types...>::_S_trivial_copy_assign,
+	_Copy_assign_base<_Traits<_Types...>::_S_trivial_copy_assign
+			  && _Traits<_Types...>::_S_trivial_copy_ctor,
 			  _Types...>;
 
   template<bool, typename... _Types>
@@ -600,21 +615,25 @@ namespace __variant
       operator=(_Move_assign_base&& __rhs)
 	  noexcept(_Traits<_Types...>::_S_nothrow_move_assign)
       {
-	if (this->_M_index == __rhs._M_index)
+	__do_visit([this, &__rhs](auto&& __this_mem, auto&& __rhs_mem) mutable
+		   -> __detail::__variant::__variant_cookie
 	  {
-	    if (__rhs._M_valid())
+	    if constexpr (is_same_v<
+			    remove_reference_t<decltype(__this_mem)>,
+			    remove_reference_t<decltype(__rhs_mem)>>)
 	      {
-		static constexpr void (*_S_vtable[])(void*, void*) =
-		  { &__erased_assign<_Types&, _Types&&>... };
-		_S_vtable[__rhs._M_index]
-		  (this->_M_storage(), __rhs._M_storage());
+		if constexpr (!is_same_v<
+			        remove_reference_t<decltype(__rhs_mem)>,
+			        __variant_cookie>)
+		  __this_mem = std::move(__rhs_mem);
 	      }
-	  }
-	else
-	  {
-	    _Move_assign_base __tmp(std::move(__rhs));
-	    this->_M_destructive_move(std::move(__tmp));
-	  }
+	    else
+	      {
+		_Move_assign_base __tmp(std::move(__rhs));
+		this->_M_destructive_move(std::move(__tmp));
+	      }
+	  return {};
+	}, __variant_cast<_Types...>(*this), __variant_cast<_Types...>(__rhs));
 	__glibcxx_assert(this->_M_index == __rhs._M_index);
 	return *this;
       }
@@ -733,15 +752,21 @@ namespace __variant
       _Tp _M_data;
     };
 
-  template<typename _Tp, size_t __first, size_t... __rest>
-    struct _Multi_array<_Tp, __first, __rest...>
+  template<typename _Ret,
+	   typename _Visitor,
+	   typename... _Variants,
+	   size_t __first, size_t... __rest>
+    struct _Multi_array<_Ret(*)(_Visitor, _Variants...), __first, __rest...>
     {
+      static constexpr int __do_cookie =
+	is_same_v<_Ret, __variant_cookie> ? 1 : 0;
+      using _Tp = _Ret(*)(_Visitor, _Variants...);
       template<typename... _Args>
 	constexpr const _Tp&
 	_M_access(size_t __first_index, _Args... __rest_indices) const
-	{ return _M_arr[__first_index]._M_access(__rest_indices...); }
+        { return _M_arr[__first_index + __do_cookie]._M_access(__rest_indices...); }
 
-      _Multi_array<_Tp, __rest...> _M_arr[__first];
+      _Multi_array<_Tp, __rest...> _M_arr[__first + __do_cookie];
     };
 
   // Creates a multi-dimensional vtable recursively.
@@ -801,18 +826,37 @@ namespace __variant
 	_S_apply_all_alts(_Array_type& __vtable,
 			  std::index_sequence<__var_indices...>)
 	{
-	  (_S_apply_single_alt<__var_indices>(
-	     __vtable._M_arr[__var_indices]), ...);
+	  if constexpr (is_same_v<_Result_type, __variant_cookie>)
+	    (_S_apply_single_alt<true, __var_indices>(
+	      __vtable._M_arr[__var_indices + 1],
+	      &(__vtable._M_arr[0])), ...);
+	  else
+	    (_S_apply_single_alt<false, __var_indices>(
+	      __vtable._M_arr[__var_indices]), ...);
 	}
 
-      template<size_t __index, typename _Tp>
+      template<bool __do_cookie, size_t __index, typename _Tp>
 	static constexpr void
-	_S_apply_single_alt(_Tp& __element)
+	_S_apply_single_alt(_Tp& __element, _Tp* __cookie_element = nullptr)
 	{
 	  using _Alternative = variant_alternative_t<__index, _Next>;
-	  __element = __gen_vtable_impl<
-	    remove_reference_t<decltype(__element)>, tuple<_Variants...>,
-	    std::index_sequence<__indices..., __index>>::_S_apply();
+	  if constexpr (__do_cookie)
+	    {
+	      __element = __gen_vtable_impl<
+		_Tp,
+		tuple<_Variants...>,
+		std::index_sequence<__indices..., __index>>::_S_apply();
+	      *__cookie_element = __gen_vtable_impl<
+		_Tp,
+		tuple<_Variants...>,
+		std::index_sequence<__indices..., variant_npos>>::_S_apply();
+	    }
+	  else
+	    {
+	      __element = __gen_vtable_impl<
+		remove_reference_t<decltype(__element)>, tuple<_Variants...>,
+		std::index_sequence<__indices..., __index>>::_S_apply();
+	    }
 	}
     };
 
@@ -825,11 +869,22 @@ namespace __variant
       using _Array_type =
 	  _Multi_array<_Result_type (*)(_Visitor&&, _Variants...)>;
 
+      template<size_t __index, typename _Variant>
+	static constexpr decltype(auto)
+	__element_by_index_or_cookie(_Variant&& __var)
+        {
+	  if constexpr (__index != variant_npos)
+	    return __variant::__get<__index>(std::forward<_Variant>(__var));
+	  else
+	    return __variant_cookie{};
+	}
+
       static constexpr decltype(auto)
       __visit_invoke(_Visitor&& __visitor, _Variants... __vars)
       {
 	return std::__invoke(std::forward<_Visitor>(__visitor),
-	    __variant::__get<__indices>(std::forward<_Variants>(__vars))...);
+	    __element_by_index_or_cookie<__indices>(
+	      std::forward<_Variants>(__vars))...);
       }
 
       static constexpr auto
@@ -843,7 +898,9 @@ namespace __variant
       using _Func_ptr = _Result_type (*)(_Visitor&&, _Variants...);
       using _Array_type =
 	  _Multi_array<_Func_ptr,
-		       variant_size_v<remove_reference_t<_Variants>>...>;
+		       (variant_size_v<remove_reference_t<_Variants>>
+			+ (is_same_v<_Result_type, __variant_cookie> ? 1 : 0))
+	               ...>;
 
       static constexpr _Array_type
       _S_apply()
@@ -869,6 +926,15 @@ namespace __variant
 } // namespace __variant
 } // namespace __detail
 
+  template<size_t _Np, typename _Variant, typename... _Args>
+    void __variant_construct_by_index(_Variant& __v, _Args&&... __args)
+    {
+      __v._M_index = _Np;
+      auto&& __storage = __detail::__variant::__get<_Np>(__v);
+      ::new (&__storage) remove_reference_t<decltype(__storage)>
+        (std::forward<_Args>(__args)...);
+    }
+
   template<typename _Tp, typename... _Types>
     constexpr bool
     holds_alternative(const variant<_Types...>& __v) noexcept
@@ -925,7 +991,7 @@ namespace __variant
 		    "The index should be in [0, number of alternatives)");
       static_assert(!is_void_v<_Alternative_type>, "_Tp should not be void");
       if (__ptr && __ptr->index() == _Np)
-	return &__detail::__variant::__get<_Np>(*__ptr);
+	return std::addressof(__detail::__variant::__get<_Np>(*__ptr));
       return nullptr;
     }
 
@@ -939,7 +1005,7 @@ namespace __variant
 		    "The index should be in [0, number of alternatives)");
       static_assert(!is_void_v<_Alternative_type>, "_Tp should not be void");
       if (__ptr && __ptr->index() == _Np)
-	return &__detail::__variant::__get<_Np>(*__ptr);
+	return std::addressof(__detail::__variant::__get<_Np>(*__ptr));
       return nullptr;
     }
 
@@ -973,7 +1039,27 @@ namespace __variant
     constexpr bool operator __OP(const variant<_Types...>& __lhs, \
 				 const variant<_Types...>& __rhs) \
     { \
-      return __lhs._M_##__NAME(__rhs, std::index_sequence_for<_Types...>{}); \
+      bool __ret = true; \
+      __do_visit([&__ret, &__lhs, __rhs] \
+		 (auto&& __this_mem, auto&& __rhs_mem) mutable	\
+		   -> __detail::__variant::__variant_cookie \
+        { \
+	  if constexpr (!is_same_v< \
+			  remove_reference_t<decltype(__this_mem)>, \
+			  remove_reference_t<decltype(__rhs_mem)>> \
+			|| is_same_v<decltype(__this_mem), \
+			             __detail::__variant::__variant_cookie>) \
+	    __ret = (__lhs.index() + 1) __OP (__rhs.index() + 1); \
+	  else if constexpr (is_same_v< \
+			       remove_reference_t<decltype(__this_mem)>, \
+			       remove_reference_t<decltype(__rhs_mem)>> \
+                             && !is_same_v< \
+	                          remove_reference_t<decltype(__this_mem)>, \
+				  __detail::__variant::__variant_cookie>) \
+	    __ret = __this_mem __OP __rhs_mem; \
+	  return {}; \
+	}, __lhs, __rhs); \
+      return __ret; \
     } \
 \
   constexpr bool operator __OP(monostate, monostate) noexcept \
@@ -1036,6 +1122,12 @@ namespace __variant
 	variant<_Types...>>
     {
     private:
+      template <typename... _UTypes, typename _Tp>
+	friend decltype(auto) __variant_cast(_Tp&&);
+      template<size_t _Np, typename _Variant, typename... _Args>
+	friend void __variant_construct_by_index(_Variant& __v,
+						 _Args&&... __args);
+
       static_assert(sizeof...(_Types) > 0,
 		    "variant must have at least one alternative");
       static_assert(!(std::is_reference_v<_Types> || ...),
@@ -1185,7 +1277,6 @@ namespace __variant
 	{
 	  static_assert(_Np < sizeof...(_Types),
 			"The index should be in [0, number of alternatives)");
-
 	  using type = variant_alternative_t<_Np, variant>;
 	  // If constructing the value can throw but move assigning it can't,
 	  // construct in a temporary and then move assign from it. This gives
@@ -1202,11 +1293,11 @@ namespace __variant
 	      return std::get<_Np>(*this);
 	    }
 
-	  this->~variant();
+	  this->_M_reset();
 	  __try
 	    {
-	      ::new (this) variant(in_place_index<_Np>,
-				   std::forward<_Args>(__args)...);
+	      __variant_construct_by_index<_Np>(*this,
+	        std::forward<_Args>(__args)...);
 	    }
 	  __catch (...)
 	    {
@@ -1225,7 +1316,6 @@ namespace __variant
 	{
 	  static_assert(_Np < sizeof...(_Types),
 			"The index should be in [0, number of alternatives)");
-
 	  using type = variant_alternative_t<_Np, variant>;
 	  if constexpr (is_trivially_copyable_v<type>
 	      && !is_nothrow_constructible_v<type, initializer_list<_Up>,
@@ -1239,11 +1329,11 @@ namespace __variant
 	      return std::get<_Np>(*this);
 	    }
 
-	  this->~variant();
+	  this->_M_reset();
 	  __try
 	    {
-	      ::new (this) variant(in_place_index<_Np>, __il,
-				   std::forward<_Args>(__args)...);
+	      __variant_construct_by_index<_Np>(*this, __il,
+		std::forward<_Args>(__args)...);
 	    }
 	  __catch (...)
 	    {
@@ -1270,62 +1360,49 @@ namespace __variant
       noexcept((__is_nothrow_swappable<_Types>::value && ...)
 	       && is_nothrow_move_constructible_v<variant>)
       {
-	if (this->index() == __rhs.index())
+	__do_visit([this, &__rhs](auto&& __this_mem, auto&& __rhs_mem) mutable
+		   -> __detail::__variant::__variant_cookie
 	  {
-	    if (this->_M_valid())
+	    if constexpr (is_same_v<
+			    remove_reference_t<decltype(__this_mem)>,
+			    remove_reference_t<decltype(__rhs_mem)>>)
 	      {
-		static constexpr void (*_S_vtable[])(void*, void*) =
-		  { &__detail::__variant::__erased_swap<_Types&, _Types&>... };
-		_S_vtable[__rhs._M_index](this->_M_storage(),
-					  __rhs._M_storage());
+		if constexpr (!is_same_v<
+			        remove_reference_t<decltype(__rhs_mem)>,
+			        __detail::__variant::__variant_cookie>)
+		  {
+		    using std::swap;
+		    swap(__this_mem, __rhs_mem);
+		  }
 	      }
-	  }
-	else if (!this->_M_valid())
-	  {
-	    this->_M_destructive_move(std::move(__rhs));
-	    __rhs._M_reset();
-	  }
-	else if (!__rhs._M_valid())
-	  {
-	    __rhs._M_destructive_move(std::move(*this));
-	    this->_M_reset();
-	  }
-	else
-	  {
-	    auto __tmp = std::move(__rhs);
-	    __rhs._M_destructive_move(std::move(*this));
-	    this->_M_destructive_move(std::move(__tmp));
-	  }
+	    else
+	      {
+		if constexpr (is_same_v<
+			        remove_reference_t<decltype(__this_mem)>,
+			        __detail::__variant::__variant_cookie>)
+		  {
+		    this->_M_destructive_move(std::move(__rhs));
+		    __rhs._M_reset();
+		  }
+		else if constexpr (is_same_v<
+			             remove_reference_t<decltype(__rhs_mem)>,
+			             __detail::__variant::__variant_cookie>)
+		  {
+		    __rhs._M_destructive_move(std::move(*this));
+		    this->_M_reset();
+		  }
+		else
+		  {
+		    auto __tmp = std::move(__rhs);
+		    __rhs._M_destructive_move(std::move(*this));
+		    this->_M_destructive_move(std::move(__tmp));
+		  }
+	      }
+	  return {};
+	}, *this, __rhs);
       }
 
     private:
-#define _VARIANT_RELATION_FUNCTION_TEMPLATE(__OP, __NAME) \
-      template<size_t... __indices> \
-	static constexpr bool \
-	(*_S_erased_##__NAME[])(const variant&, const variant&) = \
-	  { &__detail::__variant::__erased_##__NAME< \
-		const variant&, __indices>... }; \
-      template<size_t... __indices> \
-	constexpr bool \
-	_M_##__NAME(const variant& __rhs, \
-		    std::index_sequence<__indices...>) const \
-	{ \
-	  auto __lhs_index = this->index(); \
-	  auto __rhs_index = __rhs.index(); \
-	  if (__lhs_index != __rhs_index || valueless_by_exception()) \
-	    /* Modulo addition. */ \
-	    return __lhs_index + 1 __OP __rhs_index + 1; \
-	  return _S_erased_##__NAME<__indices...>[__lhs_index](*this, __rhs); \
-	}
-
-      _VARIANT_RELATION_FUNCTION_TEMPLATE(<, less)
-      _VARIANT_RELATION_FUNCTION_TEMPLATE(<=, less_equal)
-      _VARIANT_RELATION_FUNCTION_TEMPLATE(==, equal)
-      _VARIANT_RELATION_FUNCTION_TEMPLATE(!=, not_equal)
-      _VARIANT_RELATION_FUNCTION_TEMPLATE(>=, greater_equal)
-      _VARIANT_RELATION_FUNCTION_TEMPLATE(>, greater)
-
-#undef _VARIANT_RELATION_FUNCTION_TEMPLATE
 
 #if defined(__clang__) && __clang_major__ <= 7
     public:
@@ -1401,11 +1478,8 @@ namespace __variant
 
   template<typename _Visitor, typename... _Variants>
     constexpr decltype(auto)
-    visit(_Visitor&& __visitor, _Variants&&... __variants)
+    __do_visit(_Visitor&& __visitor, _Variants&&... __variants)
     {
-      if ((__variants.valueless_by_exception() || ...))
-	__throw_bad_variant_access("Unexpected index");
-
       using _Result_type =
 	decltype(std::forward<_Visitor>(__visitor)(
 	    std::get<0>(std::forward<_Variants>(__variants))...));
@@ -1418,6 +1492,17 @@ namespace __variant
 			   std::forward<_Variants>(__variants)...);
     }
 
+  template<typename _Visitor, typename... _Variants>
+    constexpr decltype(auto)
+    visit(_Visitor&& __visitor, _Variants&&... __variants)
+    {
+      if ((__variants.valueless_by_exception() || ...))
+	__throw_bad_variant_access("Unexpected index");
+
+      return __do_visit(std::forward<_Visitor>(__visitor),
+			std::forward<_Variants>(__variants)...);
+    }
+
   template<bool, typename... _Types>
     struct __variant_hash_call_base_impl
     {
@@ -1425,15 +1510,20 @@ namespace __variant
       operator()(const variant<_Types...>& __t) const
       noexcept((is_nothrow_invocable_v<hash<decay_t<_Types>>, _Types> && ...))
       {
-	if (!__t.valueless_by_exception())
+	size_t __ret;
+	__do_visit([&__t, &__ret](auto&& __t_mem) mutable
+		   -> __detail::__variant::__variant_cookie
 	  {
-	    namespace __edv = __detail::__variant;
-	    static constexpr size_t (*_S_vtable[])(void*) =
-	      { &__edv::__erased_hash<const _Types&>... };
-	    return hash<size_t>{}(__t.index())
-	      + _S_vtable[__t.index()](__edv::__get_storage(__t));
-	  }
-	return hash<size_t>{}(__t.index());
+	    using _Type = __remove_cvref_t<decltype(__t_mem)>;
+	    if constexpr (!is_same_v<_Type,
+			             __detail::__variant::__variant_cookie>)
+	      __ret = std::hash<size_t>{}(__t.index())
+		      + std::hash<_Type>{}(__t_mem);
+	    else
+	      __ret = std::hash<size_t>{}(__t.index());
+	    return {};
+	  }, __t);
+	return __ret;
       }
     };
 
diff --git a/libstdc++-v3/testsuite/20_util/variant/compile.cc b/libstdc++-v3/testsuite/20_util/variant/compile.cc
index 8a43092..04fef0b 100644
--- a/libstdc++-v3/testsuite/20_util/variant/compile.cc
+++ b/libstdc++-v3/testsuite/20_util/variant/compile.cc
@@ -488,12 +488,12 @@ void test_triviality()
   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, =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,  true,  true)
-  TEST_TEMPLATE(=default,         ,         , =default,         , false, false,  true, false)
+  TEST_TEMPLATE(=default,         ,         , =default, =default, false, false,  false,  true)
+  TEST_TEMPLATE(=default,         ,         , =default,         , false, false,  false, 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)
diff --git a/libstdc++-v3/testsuite/20_util/variant/run.cc b/libstdc++-v3/testsuite/20_util/variant/run.cc
index c5ea7df..7ee9b08 100644
--- a/libstdc++-v3/testsuite/20_util/variant/run.cc
+++ b/libstdc++-v3/testsuite/20_util/variant/run.cc
@@ -88,6 +88,21 @@ void arbitrary_ctor()
   VERIFY(get<1>(v) == "a");
 }
 
+struct ThrowingMoveCtorThrowsCopyCtor
+{
+  ThrowingMoveCtorThrowsCopyCtor() noexcept = default;
+  ThrowingMoveCtorThrowsCopyCtor(ThrowingMoveCtorThrowsCopyCtor&&) {}
+  ThrowingMoveCtorThrowsCopyCtor(ThrowingMoveCtorThrowsCopyCtor const&)
+  {
+    throw 0;
+  }
+
+  ThrowingMoveCtorThrowsCopyCtor& operator=(ThrowingMoveCtorThrowsCopyCtor&&) noexcept
+    = default;
+  ThrowingMoveCtorThrowsCopyCtor& operator=(ThrowingMoveCtorThrowsCopyCtor const&) noexcept
+    = default;
+};
+
 void copy_assign()
 {
   variant<monostate, string> v("a");
@@ -96,6 +111,20 @@ void copy_assign()
   u = v;
   VERIFY(holds_alternative<string>(u));
   VERIFY(get<string>(u) == "a");
+  {
+    std::variant<int, ThrowingMoveCtorThrowsCopyCtor> v1,
+      v2 = ThrowingMoveCtorThrowsCopyCtor();
+    bool should_throw = false;
+    try
+      {
+	v1 = v2;
+      }
+    catch(int)
+      {
+	should_throw = true;
+      }
+    VERIFY(should_throw);
+  }
 }
 
 void move_assign()
@@ -183,11 +212,15 @@ void emplace()
     AlwaysThrow a;
     try { v.emplace<1>(a); } catch (nullptr_t) { }
     VERIFY(v.valueless_by_exception());
+    v.emplace<0>(42);
+    VERIFY(!v.valueless_by_exception());
   }
   {
     variant<int, AlwaysThrow> v;
     try { v.emplace<1>(AlwaysThrow{}); } catch (nullptr_t) { }
     VERIFY(v.valueless_by_exception());
+    v.emplace<0>(42);
+    VERIFY(!v.valueless_by_exception());
   }
   VERIFY(&v.emplace<0>(1) == &std::get<0>(v));
   VERIFY(&v.emplace<int>(1) == &std::get<int>(v));
@@ -258,6 +291,7 @@ void test_relational()
     VERIFY(v < w);
     VERIFY(v <= w);
     VERIFY(!(v == w));
+    VERIFY(v == v);
     VERIFY(v != w);
     VERIFY(w > v);
     VERIFY(w >= v);
Jonathan Wakely March 6, 2019, 9:33 a.m. | #8
On 05/03/19 23:27 +0200, Ville Voutilainen wrote:
>On Mon, 4 Mar 2019 at 01:43, Ville Voutilainen

><ville.voutilainen@gmail.com> wrote:

>>

>> On Mon, 4 Mar 2019 at 01:26, Ville Voutilainen

>> <ville.voutilainen@gmail.com> wrote:

>> > I consider variant to no longer be in the state of sin after this.

>>

>> Sigh, except for the places where it self-destructs or placement-news

>> over things that it shouldn't. That's hopefully

>> the next bit that I'll rectify, Real Soon Now.

>

>Here we go. Variant, go forth and sin no more. Tested locally on

>linux-x64, will run the full suite

>on linux-ppc64. OK for trunk if tests pass?

>

>2019-03-05  Ville Voutilainen  <ville.voutilainen@gmail.com>

>

>    Rewrite variant.

>    Also PR libstdc++/85517

>    * include/std/variant (__do_visit): New.

>    (__variant_cast): Likewise.

>    (__variant_cookie): Likewise.

>    (__erased_*): Remove.

>    (_Variant_storage::_S_vtable): Likewise.

>    (_Variant_storage::__M_reset_impl): Adjust to use __do_visit.

>    (_Variant_storage::__M_reset): Adjust.

>    (__variant_construct): New.

>    (_Copy_ctor_base(const _Copy_ctor_base&)): Adjust to use

>    __variant_construct.

>    (_Move_ctor_base(_Move_ctor_base&&)): Likewise.

>    (_Move_ctor_base::__M_destructive_copy): New.

>    (_Move_ctor_base::__M_destructive_move): Adjust to use

>    __variant_construct.

>    (_Copy_assign_base::operator=): Adjust to use __do_visit.

>    (_Copy_assign_alias): Adjust to check both copy assignment

>    and copy construction for triviality.

>    (_Move_assign_base::operator=): Adjust to use __do_visit.

>    (_Multi_array): Add support for visitors that accept and return

>    a __variant_cookie.

>    (__gen_vtable_impl::_S_apply_all_alts): Likewise.

>    (__gen_vtable_impl::_S_apply_single_alt): Likewise.

>    (__gen_vtable_impl::__element_by_index_or_cookie): New. Generate

>    a __variant_cookie temporary for a variant that is valueless and..

>    (__gen_vtable_impl::__visit_invoke): ..adjust here.

>    (__gen_vtable::_Array_type): Conditionally make space for

>    the __variant_cookie visitor case.

>    (__variant_construct_by_index): New.

>    (get_if): Adjust to use std::addressof.

>    (relops): Adjust to use __do_visit.

>    (variant): Add __variant_cast and __variant_construct_by_index

>    as friends.

>    (variant::emplace): Use _M_reset() and __variant_construct_by_index

>    instead of self-destruction.

>    (variant::swap): Adjust to use __do_visit.

>    (visit): Reimplement in terms of __do_visit.

>    (__variant_hash_call_base_impl::operator()): Adjust to use __do_visit.

>    * testsuite/20_util/variant/compile.cc: Adjust.

>    * testsuite/20_util/variant/run.cc: Likewise.


>diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant

>index 89deb14..5138599 100644

>--- a/libstdc++-v3/include/std/variant

>+++ b/libstdc++-v3/include/std/variant

>@@ -138,6 +138,21 @@ namespace __variant

>     constexpr variant_alternative_t<_Np, variant<_Types...>> const&&

>     get(const variant<_Types...>&&);

> 

>+  template<typename _Visitor, typename... _Variants>

>+    constexpr decltype(auto)

>+    __do_visit(_Visitor&& __visitor, _Variants&&... __variants);

>+

>+  template <typename... _Types, typename _Tp>

>+    decltype(auto) __variant_cast(_Tp&& __rhs)

>+    {

>+      if constexpr (is_const_v<remove_reference_t<_Tp>>)

>+        return static_cast<const variant<_Types...>&>(__rhs);

>+      else if constexpr (is_rvalue_reference_v<_Tp&&>)


I know what this is doing, but it still looks a little odd to ask if
T&& is an rvalue-reference.

Would it be clearer to structure this as:

      if constexpr (is_lvalue_reference_v<_Tp>)
        {
          if constexpr (is_const_v<remove_reference_t<_Tp>>)
            return static_cast<const variant<_Types...>&>(__rhs);
          else
            return static_cast<variant<_Types...>&>(__rhs);
        }
      else
        return static_cast<variant<_Types...>&&>(__rhs);

?

>+  template<typename... _Types, typename _Tp, typename _Up>

>+    void __variant_construct(_Tp&& __lhs, _Up&& __rhs)

>+    {

>+      __lhs._M_index = __rhs._M_index;

>+      __do_visit([](auto&& __this_mem, auto&& __rhs_mem) mutable

>+		 -> __detail::__variant::__variant_cookie

>+        {

>+	  using _Type = remove_reference_t<decltype(__this_mem)>;

>+	  if constexpr (is_same_v<__remove_cvref_t<decltype(__rhs_mem)>,

>+			          remove_cv_t<_Type>>

>+			&& !is_same_v<_Type, __variant_cookie>)

>+	    ::new (std::addressof(__this_mem))


Is there any way that this can find the wrong operator new?

Even if it can't, saying ::new ((void*)std::addressof(__this_mem))
would avoid having to think about that question again in future.

Therre are a few other new expressions where that applies too (several
of them already there before your patch).

>@@ -869,6 +926,15 @@ namespace __variant

> } // namespace __variant

> } // namespace __detail

> 

>+  template<size_t _Np, typename _Variant, typename... _Args>

>+    void __variant_construct_by_index(_Variant& __v, _Args&&... __args)

>+    {

>+      __v._M_index = _Np;

>+      auto&& __storage = __detail::__variant::__get<_Np>(__v);

>+      ::new (&__storage) remove_reference_t<decltype(__storage)>


This one definitely needs to be cast to void* and needs to use
addressof (or __addressof), otherwise ...

struct X {
  ~X() { } // not trivially copyable
  X* operator&() { puts("First problem"); return this; }
};

void* operator new(std::size_t, X*)
{
  puts("second problem");
  abort();
}

int main()
{
  std::variant <X> v;
  v.emplace<0>();
}
Ville Voutilainen March 6, 2019, 9:56 a.m. | #9
On Wed, 6 Mar 2019 at 11:33, Jonathan Wakely <jwakely@redhat.com> wrote:
> >+      else if constexpr (is_rvalue_reference_v<_Tp&&>)

>

> I know what this is doing, but it still looks a little odd to ask if

> T&& is an rvalue-reference.

>

> Would it be clearer to structure this as:

>

>       if constexpr (is_lvalue_reference_v<_Tp>)

>         {

>           if constexpr (is_const_v<remove_reference_t<_Tp>>)

>             return static_cast<const variant<_Types...>&>(__rhs);

>           else

>             return static_cast<variant<_Types...>&>(__rhs);

>         }

>       else

>         return static_cast<variant<_Types...>&&>(__rhs);

>

> ?

> >+          ::new (std::addressof(__this_mem))

>

> Is there any way that this can find the wrong operator new?

>

> Even if it can't, saying ::new ((void*)std::addressof(__this_mem))

> would avoid having to think about that question again in future.

>

> Therre are a few other new expressions where that applies too (several

> of them already there before your patch).

> >+      ::new (&__storage) remove_reference_t<decltype(__storage)>

>

> This one definitely needs to be cast to void* and needs to use

> addressof (or __addressof), otherwise ...



Sure thing; an incremental diff attached.
diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 5138599..4d81ceb 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -145,12 +145,15 @@ namespace __variant
   template <typename... _Types, typename _Tp>
     decltype(auto) __variant_cast(_Tp&& __rhs)
     {
-      if constexpr (is_const_v<remove_reference_t<_Tp>>)
-        return static_cast<const variant<_Types...>&>(__rhs);
-      else if constexpr (is_rvalue_reference_v<_Tp&&>)
-        return static_cast<variant<_Types...>&&>(__rhs);
+      if constexpr (is_lvalue_reference_v<_Tp>)
+	{
+	  if constexpr (is_const_v<remove_reference_t<_Tp>>)
+			 return static_cast<const variant<_Types...>&>(__rhs);
+	  else
+	    return static_cast<variant<_Types...>&>(__rhs);
+	}
       else
-        return static_cast<variant<_Types...>&>(__rhs);
+        return static_cast<variant<_Types...>&&>(__rhs);
     }
 
 namespace __detail
@@ -212,7 +215,8 @@ namespace __variant
     {
       template<typename... _Args>
       constexpr _Uninitialized(in_place_index_t<0>, _Args&&... __args)
-      { ::new (&_M_storage) _Type(std::forward<_Args>(__args)...); }
+      { ::new ((void*)std::addressof(_M_storage))
+	  _Type(std::forward<_Args>(__args)...); }
 
       const _Type& _M_get() const &
       { return *_M_storage._M_ptr(); }
@@ -427,7 +431,7 @@ namespace __variant
 	  if constexpr (is_same_v<__remove_cvref_t<decltype(__rhs_mem)>,
 			          remove_cv_t<_Type>>
 			&& !is_same_v<_Type, __variant_cookie>)
-	    ::new (std::addressof(__this_mem))
+	    ::new ((void*)std::addressof(__this_mem))
 	      _Type(std::forward<decltype(__rhs_mem)>(__rhs_mem));
 	  return {};
 	}, __variant_cast<_Types...>(__lhs),
@@ -931,8 +935,9 @@ namespace __variant
     {
       __v._M_index = _Np;
       auto&& __storage = __detail::__variant::__get<_Np>(__v);
-      ::new (&__storage) remove_reference_t<decltype(__storage)>
-        (std::forward<_Args>(__args)...);
+      ::new ((void*)std::addressof(__storage))
+        remove_reference_t<decltype(__storage)>
+	  (std::forward<_Args>(__args)...);
     }
 
   template<typename _Tp, typename... _Types>
Ville Voutilainen March 6, 2019, 9:57 a.m. | #10
On Wed, 6 Mar 2019 at 11:56, Ville Voutilainen
<ville.voutilainen@gmail.com> wrote:
> > This one definitely needs to be cast to void* and needs to use

> > addressof (or __addressof), otherwise ...

>

>

> Sure thing; an incremental diff attached.


And here's the whole shebang.
diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 89deb14..4d81ceb 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -138,6 +138,24 @@ namespace __variant
     constexpr variant_alternative_t<_Np, variant<_Types...>> const&&
     get(const variant<_Types...>&&);
 
+  template<typename _Visitor, typename... _Variants>
+    constexpr decltype(auto)
+    __do_visit(_Visitor&& __visitor, _Variants&&... __variants);
+
+  template <typename... _Types, typename _Tp>
+    decltype(auto) __variant_cast(_Tp&& __rhs)
+    {
+      if constexpr (is_lvalue_reference_v<_Tp>)
+	{
+	  if constexpr (is_const_v<remove_reference_t<_Tp>>)
+			 return static_cast<const variant<_Types...>&>(__rhs);
+	  else
+	    return static_cast<variant<_Types...>&>(__rhs);
+	}
+      else
+        return static_cast<variant<_Types...>&&>(__rhs);
+    }
+
 namespace __detail
 {
 namespace __variant
@@ -155,6 +173,9 @@ namespace __variant
       std::integral_constant<size_t, is_same_v<_Tp, _First>
 	? 0 : __index_of_v<_Tp, _Rest...> + 1> {};
 
+  // used for raw visitation
+  struct __variant_cookie {};
+
   // _Uninitialized<T> is guaranteed to be a literal type, even if T is not.
   // We have to do this, because [basic.types]p10.5.3 (n4606) is not implemented
   // yet. When it's implemented, _Uninitialized<T> can be changed to the alias
@@ -194,7 +215,8 @@ namespace __variant
     {
       template<typename... _Args>
       constexpr _Uninitialized(in_place_index_t<0>, _Args&&... __args)
-      { ::new (&_M_storage) _Type(std::forward<_Args>(__args)...); }
+      { ::new ((void*)std::addressof(_M_storage))
+	  _Type(std::forward<_Args>(__args)...); }
 
       const _Type& _M_get() const &
       { return *_M_storage._M_ptr(); }
@@ -236,63 +258,6 @@ namespace __variant
 			      std::forward<_Variant>(__v)._M_u);
     }
 
-  // Various functions as "vtable" entries, where those vtables are used by
-  // polymorphic operations.
-  template<typename _Lhs, typename _Rhs>
-    void
-    __erased_ctor(void* __lhs, void* __rhs)
-    {
-      using _Type = remove_reference_t<_Lhs>;
-      ::new (__lhs) _Type(__variant::__ref_cast<_Rhs>(__rhs));
-    }
-
-  template<typename _Variant, size_t _Np>
-    void
-    __erased_dtor(_Variant&& __v)
-    { std::_Destroy(std::__addressof(__variant::__get<_Np>(__v))); }
-
-  template<typename _Lhs, typename _Rhs>
-    void
-    __erased_assign(void* __lhs, void* __rhs)
-    {
-      __variant::__ref_cast<_Lhs>(__lhs) = __variant::__ref_cast<_Rhs>(__rhs);
-    }
-
-  template<typename _Lhs, typename _Rhs>
-    void
-    __erased_swap(void* __lhs, void* __rhs)
-    {
-      using std::swap;
-      swap(__variant::__ref_cast<_Lhs>(__lhs),
-	   __variant::__ref_cast<_Rhs>(__rhs));
-    }
-
-#define _VARIANT_RELATION_FUNCTION_TEMPLATE(__OP, __NAME) \
-  template<typename _Variant, size_t _Np> \
-    constexpr bool \
-    __erased_##__NAME(const _Variant& __lhs, const _Variant& __rhs) \
-    { \
-      return __variant::__get<_Np>(std::forward<_Variant>(__lhs)) \
-	  __OP __variant::__get<_Np>(std::forward<_Variant>(__rhs)); \
-    }
-
-  _VARIANT_RELATION_FUNCTION_TEMPLATE(<, less)
-  _VARIANT_RELATION_FUNCTION_TEMPLATE(<=, less_equal)
-  _VARIANT_RELATION_FUNCTION_TEMPLATE(==, equal)
-  _VARIANT_RELATION_FUNCTION_TEMPLATE(!=, not_equal)
-  _VARIANT_RELATION_FUNCTION_TEMPLATE(>=, greater_equal)
-  _VARIANT_RELATION_FUNCTION_TEMPLATE(>, greater)
-
-#undef _VARIANT_RELATION_FUNCTION_TEMPLATE
-
-  template<typename _Tp>
-    size_t
-    __erased_hash(void* __t)
-    {
-      return std::hash<__remove_cvref_t<_Tp>>{}(
-	  __variant::__ref_cast<_Tp>(__t));
-    }
-
   template<typename... _Types>
     struct _Traits
     {
@@ -369,9 +334,6 @@ namespace __variant
   template<typename... _Types>
     struct _Variant_storage<false, _Types...>
     {
-      template<size_t... __indices>
-	static constexpr void (*_S_vtable[])(const _Variant_storage&) =
-	    { &__erased_dtor<const _Variant_storage&, __indices>... };
 
       constexpr _Variant_storage() : _M_index(variant_npos) { }
 
@@ -381,16 +343,21 @@ namespace __variant
 	_M_index(_Np)
 	{ }
 
-      template<size_t... __indices>
-	constexpr void _M_reset_impl(std::index_sequence<__indices...>)
-	{
-	  if (_M_index != __index_type(variant_npos))
-	    _S_vtable<__indices...>[_M_index](*this);
+      constexpr void _M_reset_impl()
+      {
+	__do_visit([](auto&& __this_mem) mutable
+		   -> __detail::__variant::__variant_cookie
+	  {
+	    if constexpr (!is_same_v<remove_reference_t<decltype(__this_mem)>,
+			  __variant_cookie>)
+	      std::_Destroy(std::__addressof(__this_mem));
+	    return {};
+	  }, __variant_cast<_Types...>(*this));
 	}
 
       void _M_reset()
       {
-	_M_reset_impl(std::index_sequence_for<_Types...>{});
+	_M_reset_impl();
 	_M_index = variant_npos;
       }
 
@@ -453,6 +420,24 @@ namespace __variant
     using _Variant_storage_alias =
 	_Variant_storage<_Traits<_Types...>::_S_trivial_dtor, _Types...>;
 
+  template<typename... _Types, typename _Tp, typename _Up>
+    void __variant_construct(_Tp&& __lhs, _Up&& __rhs)
+    {
+      __lhs._M_index = __rhs._M_index;
+      __do_visit([](auto&& __this_mem, auto&& __rhs_mem) mutable
+		 -> __detail::__variant::__variant_cookie
+        {
+	  using _Type = remove_reference_t<decltype(__this_mem)>;
+	  if constexpr (is_same_v<__remove_cvref_t<decltype(__rhs_mem)>,
+			          remove_cv_t<_Type>>
+			&& !is_same_v<_Type, __variant_cookie>)
+	    ::new ((void*)std::addressof(__this_mem))
+	      _Type(std::forward<decltype(__rhs_mem)>(__rhs_mem));
+	  return {};
+	}, __variant_cast<_Types...>(__lhs),
+	   __variant_cast<_Types...>(std::forward<decltype(__rhs)>(__rhs)));
+    }
+
   // The following are (Copy|Move) (ctor|assign) layers for forwarding
   // triviality and handling non-trivial SMF behaviors.
 
@@ -465,13 +450,7 @@ namespace __variant
       _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](this->_M_storage(), __rhs._M_storage());
-	    this->_M_index = __rhs._M_index;
-	  }
+	__variant_construct<_Types...>(*this, __rhs);
       }
 
       _Copy_ctor_base(_Copy_ctor_base&&) = default;
@@ -499,21 +478,31 @@ namespace __variant
       _Move_ctor_base(_Move_ctor_base&& __rhs)
 	  noexcept(_Traits<_Types...>::_S_nothrow_move_ctor)
       {
-	if (__rhs._M_valid())
+	__variant_construct<_Types...>(*this,
+	  std::forward<_Move_ctor_base>(__rhs));
+      }
+
+      void _M_destructive_move(_Move_ctor_base&& __rhs)
+      {
+	this->_M_reset();
+	__try
 	  {
-	    static constexpr void (*_S_vtable[])(void*, void*) =
-	      { &__erased_ctor<_Types&, _Types&&>... };
-	    _S_vtable[__rhs._M_index](this->_M_storage(), __rhs._M_storage());
-	    this->_M_index = __rhs._M_index;
+	    __variant_construct<_Types...>(*this,
+	      std::forward<_Move_ctor_base>(__rhs));
+	  }
+	__catch (...)
+	  {
+	    this->_M_index = variant_npos;
+	    __throw_exception_again;
 	  }
       }
 
-      void _M_destructive_move(_Move_ctor_base&& __rhs)
+      void _M_destructive_copy(const _Move_ctor_base& __rhs)
       {
-	this->~_Move_ctor_base();
+	this->_M_reset();
 	__try
 	  {
-	    ::new (this) _Move_ctor_base(std::move(__rhs));
+	    __variant_construct<_Types...>(*this, __rhs);
 	  }
 	__catch (...)
 	  {
@@ -535,8 +524,14 @@ namespace __variant
 
       void _M_destructive_move(_Move_ctor_base&& __rhs)
       {
-	this->~_Move_ctor_base();
-	::new (this) _Move_ctor_base(std::move(__rhs));
+	this->_M_reset();
+	__variant_construct<_Types...>(*this,
+          std::forward<_Move_ctor_base>(__rhs));
+      }
+      void _M_destructive_copy(const _Move_ctor_base& __rhs)
+      {
+	this->_M_reset();
+	__variant_construct<_Types...>(*this, __rhs);
       }
     };
 
@@ -554,21 +549,44 @@ namespace __variant
       operator=(const _Copy_assign_base& __rhs)
 	  noexcept(_Traits<_Types...>::_S_nothrow_copy_assign)
       {
-	if (this->_M_index == __rhs._M_index)
+	__do_visit([this, &__rhs](auto&& __this_mem, auto&& __rhs_mem) mutable
+		   -> __detail::__variant::__variant_cookie
 	  {
-	    if (__rhs._M_valid())
+	    if constexpr (is_same_v<
+			    remove_reference_t<decltype(__this_mem)>,
+			    remove_reference_t<decltype(__rhs_mem)>>)
 	      {
-		static constexpr void (*_S_vtable[])(void*, void*) =
-		  { &__erased_assign<_Types&, const _Types&>... };
-		_S_vtable[__rhs._M_index](this->_M_storage(),
-					  __rhs._M_storage());
+		if constexpr (!is_same_v<
+			        remove_reference_t<decltype(__rhs_mem)>,
+			        __variant_cookie>)
+		  __this_mem = __rhs_mem;
 	      }
-	  }
-	else
-	  {
-	    _Copy_assign_base __tmp(__rhs);
-	    this->_M_destructive_move(std::move(__tmp));
-	  }
+	    else
+	      {
+		if constexpr (!is_same_v<
+			        remove_reference_t<decltype(__rhs_mem)>,
+			        __variant_cookie>)
+		  {
+		    using __rhs_type =
+		      remove_reference_t<decltype(__rhs_mem)>;
+		    if constexpr (is_nothrow_copy_constructible_v<__rhs_type>
+		      || !is_nothrow_move_constructible_v<__rhs_type>)
+		      {
+			this->_M_destructive_copy(__rhs);
+		      }
+		    else
+		      {
+			_Copy_assign_base __tmp(__rhs);
+			this->_M_destructive_move(std::move(__tmp));
+		      }
+		  }
+		else
+		  {
+		    this->_M_reset();
+		  }
+	      }
+	  return {};
+	}, __variant_cast<_Types...>(*this), __variant_cast<_Types...>(__rhs));
 	__glibcxx_assert(this->_M_index == __rhs._M_index);
 	return *this;
       }
@@ -587,7 +605,8 @@ namespace __variant
 
   template<typename... _Types>
     using _Copy_assign_alias =
-	_Copy_assign_base<_Traits<_Types...>::_S_trivial_copy_assign,
+	_Copy_assign_base<_Traits<_Types...>::_S_trivial_copy_assign
+			  && _Traits<_Types...>::_S_trivial_copy_ctor,
 			  _Types...>;
 
   template<bool, typename... _Types>
@@ -600,21 +619,25 @@ namespace __variant
       operator=(_Move_assign_base&& __rhs)
 	  noexcept(_Traits<_Types...>::_S_nothrow_move_assign)
       {
-	if (this->_M_index == __rhs._M_index)
+	__do_visit([this, &__rhs](auto&& __this_mem, auto&& __rhs_mem) mutable
+		   -> __detail::__variant::__variant_cookie
 	  {
-	    if (__rhs._M_valid())
+	    if constexpr (is_same_v<
+			    remove_reference_t<decltype(__this_mem)>,
+			    remove_reference_t<decltype(__rhs_mem)>>)
 	      {
-		static constexpr void (*_S_vtable[])(void*, void*) =
-		  { &__erased_assign<_Types&, _Types&&>... };
-		_S_vtable[__rhs._M_index]
-		  (this->_M_storage(), __rhs._M_storage());
+		if constexpr (!is_same_v<
+			        remove_reference_t<decltype(__rhs_mem)>,
+			        __variant_cookie>)
+		  __this_mem = std::move(__rhs_mem);
 	      }
-	  }
-	else
-	  {
-	    _Move_assign_base __tmp(std::move(__rhs));
-	    this->_M_destructive_move(std::move(__tmp));
-	  }
+	    else
+	      {
+		_Move_assign_base __tmp(std::move(__rhs));
+		this->_M_destructive_move(std::move(__tmp));
+	      }
+	  return {};
+	}, __variant_cast<_Types...>(*this), __variant_cast<_Types...>(__rhs));
 	__glibcxx_assert(this->_M_index == __rhs._M_index);
 	return *this;
       }
@@ -733,15 +756,21 @@ namespace __variant
       _Tp _M_data;
     };
 
-  template<typename _Tp, size_t __first, size_t... __rest>
-    struct _Multi_array<_Tp, __first, __rest...>
+  template<typename _Ret,
+	   typename _Visitor,
+	   typename... _Variants,
+	   size_t __first, size_t... __rest>
+    struct _Multi_array<_Ret(*)(_Visitor, _Variants...), __first, __rest...>
     {
+      static constexpr int __do_cookie =
+	is_same_v<_Ret, __variant_cookie> ? 1 : 0;
+      using _Tp = _Ret(*)(_Visitor, _Variants...);
       template<typename... _Args>
 	constexpr const _Tp&
 	_M_access(size_t __first_index, _Args... __rest_indices) const
-	{ return _M_arr[__first_index]._M_access(__rest_indices...); }
+        { return _M_arr[__first_index + __do_cookie]._M_access(__rest_indices...); }
 
-      _Multi_array<_Tp, __rest...> _M_arr[__first];
+      _Multi_array<_Tp, __rest...> _M_arr[__first + __do_cookie];
     };
 
   // Creates a multi-dimensional vtable recursively.
@@ -801,18 +830,37 @@ namespace __variant
 	_S_apply_all_alts(_Array_type& __vtable,
 			  std::index_sequence<__var_indices...>)
 	{
-	  (_S_apply_single_alt<__var_indices>(
-	     __vtable._M_arr[__var_indices]), ...);
+	  if constexpr (is_same_v<_Result_type, __variant_cookie>)
+	    (_S_apply_single_alt<true, __var_indices>(
+	      __vtable._M_arr[__var_indices + 1],
+	      &(__vtable._M_arr[0])), ...);
+	  else
+	    (_S_apply_single_alt<false, __var_indices>(
+	      __vtable._M_arr[__var_indices]), ...);
 	}
 
-      template<size_t __index, typename _Tp>
+      template<bool __do_cookie, size_t __index, typename _Tp>
 	static constexpr void
-	_S_apply_single_alt(_Tp& __element)
+	_S_apply_single_alt(_Tp& __element, _Tp* __cookie_element = nullptr)
 	{
 	  using _Alternative = variant_alternative_t<__index, _Next>;
-	  __element = __gen_vtable_impl<
-	    remove_reference_t<decltype(__element)>, tuple<_Variants...>,
-	    std::index_sequence<__indices..., __index>>::_S_apply();
+	  if constexpr (__do_cookie)
+	    {
+	      __element = __gen_vtable_impl<
+		_Tp,
+		tuple<_Variants...>,
+		std::index_sequence<__indices..., __index>>::_S_apply();
+	      *__cookie_element = __gen_vtable_impl<
+		_Tp,
+		tuple<_Variants...>,
+		std::index_sequence<__indices..., variant_npos>>::_S_apply();
+	    }
+	  else
+	    {
+	      __element = __gen_vtable_impl<
+		remove_reference_t<decltype(__element)>, tuple<_Variants...>,
+		std::index_sequence<__indices..., __index>>::_S_apply();
+	    }
 	}
     };
 
@@ -825,11 +873,22 @@ namespace __variant
       using _Array_type =
 	  _Multi_array<_Result_type (*)(_Visitor&&, _Variants...)>;
 
+      template<size_t __index, typename _Variant>
+	static constexpr decltype(auto)
+	__element_by_index_or_cookie(_Variant&& __var)
+        {
+	  if constexpr (__index != variant_npos)
+	    return __variant::__get<__index>(std::forward<_Variant>(__var));
+	  else
+	    return __variant_cookie{};
+	}
+
       static constexpr decltype(auto)
       __visit_invoke(_Visitor&& __visitor, _Variants... __vars)
       {
 	return std::__invoke(std::forward<_Visitor>(__visitor),
-	    __variant::__get<__indices>(std::forward<_Variants>(__vars))...);
+	    __element_by_index_or_cookie<__indices>(
+	      std::forward<_Variants>(__vars))...);
       }
 
       static constexpr auto
@@ -843,7 +902,9 @@ namespace __variant
       using _Func_ptr = _Result_type (*)(_Visitor&&, _Variants...);
       using _Array_type =
 	  _Multi_array<_Func_ptr,
-		       variant_size_v<remove_reference_t<_Variants>>...>;
+		       (variant_size_v<remove_reference_t<_Variants>>
+			+ (is_same_v<_Result_type, __variant_cookie> ? 1 : 0))
+	               ...>;
 
       static constexpr _Array_type
       _S_apply()
@@ -869,6 +930,16 @@ namespace __variant
 } // namespace __variant
 } // namespace __detail
 
+  template<size_t _Np, typename _Variant, typename... _Args>
+    void __variant_construct_by_index(_Variant& __v, _Args&&... __args)
+    {
+      __v._M_index = _Np;
+      auto&& __storage = __detail::__variant::__get<_Np>(__v);
+      ::new ((void*)std::addressof(__storage))
+        remove_reference_t<decltype(__storage)>
+	  (std::forward<_Args>(__args)...);
+    }
+
   template<typename _Tp, typename... _Types>
     constexpr bool
     holds_alternative(const variant<_Types...>& __v) noexcept
@@ -925,7 +996,7 @@ namespace __variant
 		    "The index should be in [0, number of alternatives)");
       static_assert(!is_void_v<_Alternative_type>, "_Tp should not be void");
       if (__ptr && __ptr->index() == _Np)
-	return &__detail::__variant::__get<_Np>(*__ptr);
+	return std::addressof(__detail::__variant::__get<_Np>(*__ptr));
       return nullptr;
     }
 
@@ -939,7 +1010,7 @@ namespace __variant
 		    "The index should be in [0, number of alternatives)");
       static_assert(!is_void_v<_Alternative_type>, "_Tp should not be void");
       if (__ptr && __ptr->index() == _Np)
-	return &__detail::__variant::__get<_Np>(*__ptr);
+	return std::addressof(__detail::__variant::__get<_Np>(*__ptr));
       return nullptr;
     }
 
@@ -973,7 +1044,27 @@ namespace __variant
     constexpr bool operator __OP(const variant<_Types...>& __lhs, \
 				 const variant<_Types...>& __rhs) \
     { \
-      return __lhs._M_##__NAME(__rhs, std::index_sequence_for<_Types...>{}); \
+      bool __ret = true; \
+      __do_visit([&__ret, &__lhs, __rhs] \
+		 (auto&& __this_mem, auto&& __rhs_mem) mutable	\
+		   -> __detail::__variant::__variant_cookie \
+        { \
+	  if constexpr (!is_same_v< \
+			  remove_reference_t<decltype(__this_mem)>, \
+			  remove_reference_t<decltype(__rhs_mem)>> \
+			|| is_same_v<decltype(__this_mem), \
+			             __detail::__variant::__variant_cookie>) \
+	    __ret = (__lhs.index() + 1) __OP (__rhs.index() + 1); \
+	  else if constexpr (is_same_v< \
+			       remove_reference_t<decltype(__this_mem)>, \
+			       remove_reference_t<decltype(__rhs_mem)>> \
+                             && !is_same_v< \
+	                          remove_reference_t<decltype(__this_mem)>, \
+				  __detail::__variant::__variant_cookie>) \
+	    __ret = __this_mem __OP __rhs_mem; \
+	  return {}; \
+	}, __lhs, __rhs); \
+      return __ret; \
     } \
 \
   constexpr bool operator __OP(monostate, monostate) noexcept \
@@ -1036,6 +1127,12 @@ namespace __variant
 	variant<_Types...>>
     {
     private:
+      template <typename... _UTypes, typename _Tp>
+	friend decltype(auto) __variant_cast(_Tp&&);
+      template<size_t _Np, typename _Variant, typename... _Args>
+	friend void __variant_construct_by_index(_Variant& __v,
+						 _Args&&... __args);
+
       static_assert(sizeof...(_Types) > 0,
 		    "variant must have at least one alternative");
       static_assert(!(std::is_reference_v<_Types> || ...),
@@ -1185,7 +1282,6 @@ namespace __variant
 	{
 	  static_assert(_Np < sizeof...(_Types),
 			"The index should be in [0, number of alternatives)");
-
 	  using type = variant_alternative_t<_Np, variant>;
 	  // If constructing the value can throw but move assigning it can't,
 	  // construct in a temporary and then move assign from it. This gives
@@ -1202,11 +1298,11 @@ namespace __variant
 	      return std::get<_Np>(*this);
 	    }
 
-	  this->~variant();
+	  this->_M_reset();
 	  __try
 	    {
-	      ::new (this) variant(in_place_index<_Np>,
-				   std::forward<_Args>(__args)...);
+	      __variant_construct_by_index<_Np>(*this,
+	        std::forward<_Args>(__args)...);
 	    }
 	  __catch (...)
 	    {
@@ -1225,7 +1321,6 @@ namespace __variant
 	{
 	  static_assert(_Np < sizeof...(_Types),
 			"The index should be in [0, number of alternatives)");
-
 	  using type = variant_alternative_t<_Np, variant>;
 	  if constexpr (is_trivially_copyable_v<type>
 	      && !is_nothrow_constructible_v<type, initializer_list<_Up>,
@@ -1239,11 +1334,11 @@ namespace __variant
 	      return std::get<_Np>(*this);
 	    }
 
-	  this->~variant();
+	  this->_M_reset();
 	  __try
 	    {
-	      ::new (this) variant(in_place_index<_Np>, __il,
-				   std::forward<_Args>(__args)...);
+	      __variant_construct_by_index<_Np>(*this, __il,
+		std::forward<_Args>(__args)...);
 	    }
 	  __catch (...)
 	    {
@@ -1270,62 +1365,49 @@ namespace __variant
       noexcept((__is_nothrow_swappable<_Types>::value && ...)
 	       && is_nothrow_move_constructible_v<variant>)
       {
-	if (this->index() == __rhs.index())
+	__do_visit([this, &__rhs](auto&& __this_mem, auto&& __rhs_mem) mutable
+		   -> __detail::__variant::__variant_cookie
 	  {
-	    if (this->_M_valid())
+	    if constexpr (is_same_v<
+			    remove_reference_t<decltype(__this_mem)>,
+			    remove_reference_t<decltype(__rhs_mem)>>)
 	      {
-		static constexpr void (*_S_vtable[])(void*, void*) =
-		  { &__detail::__variant::__erased_swap<_Types&, _Types&>... };
-		_S_vtable[__rhs._M_index](this->_M_storage(),
-					  __rhs._M_storage());
+		if constexpr (!is_same_v<
+			        remove_reference_t<decltype(__rhs_mem)>,
+			        __detail::__variant::__variant_cookie>)
+		  {
+		    using std::swap;
+		    swap(__this_mem, __rhs_mem);
+		  }
 	      }
-	  }
-	else if (!this->_M_valid())
-	  {
-	    this->_M_destructive_move(std::move(__rhs));
-	    __rhs._M_reset();
-	  }
-	else if (!__rhs._M_valid())
-	  {
-	    __rhs._M_destructive_move(std::move(*this));
-	    this->_M_reset();
-	  }
-	else
-	  {
-	    auto __tmp = std::move(__rhs);
-	    __rhs._M_destructive_move(std::move(*this));
-	    this->_M_destructive_move(std::move(__tmp));
-	  }
+	    else
+	      {
+		if constexpr (is_same_v<
+			        remove_reference_t<decltype(__this_mem)>,
+			        __detail::__variant::__variant_cookie>)
+		  {
+		    this->_M_destructive_move(std::move(__rhs));
+		    __rhs._M_reset();
+		  }
+		else if constexpr (is_same_v<
+			             remove_reference_t<decltype(__rhs_mem)>,
+			             __detail::__variant::__variant_cookie>)
+		  {
+		    __rhs._M_destructive_move(std::move(*this));
+		    this->_M_reset();
+		  }
+		else
+		  {
+		    auto __tmp = std::move(__rhs);
+		    __rhs._M_destructive_move(std::move(*this));
+		    this->_M_destructive_move(std::move(__tmp));
+		  }
+	      }
+	  return {};
+	}, *this, __rhs);
       }
 
     private:
-#define _VARIANT_RELATION_FUNCTION_TEMPLATE(__OP, __NAME) \
-      template<size_t... __indices> \
-	static constexpr bool \
-	(*_S_erased_##__NAME[])(const variant&, const variant&) = \
-	  { &__detail::__variant::__erased_##__NAME< \
-		const variant&, __indices>... }; \
-      template<size_t... __indices> \
-	constexpr bool \
-	_M_##__NAME(const variant& __rhs, \
-		    std::index_sequence<__indices...>) const \
-	{ \
-	  auto __lhs_index = this->index(); \
-	  auto __rhs_index = __rhs.index(); \
-	  if (__lhs_index != __rhs_index || valueless_by_exception()) \
-	    /* Modulo addition. */ \
-	    return __lhs_index + 1 __OP __rhs_index + 1; \
-	  return _S_erased_##__NAME<__indices...>[__lhs_index](*this, __rhs); \
-	}
-
-      _VARIANT_RELATION_FUNCTION_TEMPLATE(<, less)
-      _VARIANT_RELATION_FUNCTION_TEMPLATE(<=, less_equal)
-      _VARIANT_RELATION_FUNCTION_TEMPLATE(==, equal)
-      _VARIANT_RELATION_FUNCTION_TEMPLATE(!=, not_equal)
-      _VARIANT_RELATION_FUNCTION_TEMPLATE(>=, greater_equal)
-      _VARIANT_RELATION_FUNCTION_TEMPLATE(>, greater)
-
-#undef _VARIANT_RELATION_FUNCTION_TEMPLATE
 
 #if defined(__clang__) && __clang_major__ <= 7
     public:
@@ -1401,11 +1483,8 @@ namespace __variant
 
   template<typename _Visitor, typename... _Variants>
     constexpr decltype(auto)
-    visit(_Visitor&& __visitor, _Variants&&... __variants)
+    __do_visit(_Visitor&& __visitor, _Variants&&... __variants)
     {
-      if ((__variants.valueless_by_exception() || ...))
-	__throw_bad_variant_access("Unexpected index");
-
       using _Result_type =
 	decltype(std::forward<_Visitor>(__visitor)(
 	    std::get<0>(std::forward<_Variants>(__variants))...));
@@ -1418,6 +1497,17 @@ namespace __variant
 			   std::forward<_Variants>(__variants)...);
     }
 
+  template<typename _Visitor, typename... _Variants>
+    constexpr decltype(auto)
+    visit(_Visitor&& __visitor, _Variants&&... __variants)
+    {
+      if ((__variants.valueless_by_exception() || ...))
+	__throw_bad_variant_access("Unexpected index");
+
+      return __do_visit(std::forward<_Visitor>(__visitor),
+			std::forward<_Variants>(__variants)...);
+    }
+
   template<bool, typename... _Types>
     struct __variant_hash_call_base_impl
     {
@@ -1425,15 +1515,20 @@ namespace __variant
       operator()(const variant<_Types...>& __t) const
       noexcept((is_nothrow_invocable_v<hash<decay_t<_Types>>, _Types> && ...))
       {
-	if (!__t.valueless_by_exception())
+	size_t __ret;
+	__do_visit([&__t, &__ret](auto&& __t_mem) mutable
+		   -> __detail::__variant::__variant_cookie
 	  {
-	    namespace __edv = __detail::__variant;
-	    static constexpr size_t (*_S_vtable[])(void*) =
-	      { &__edv::__erased_hash<const _Types&>... };
-	    return hash<size_t>{}(__t.index())
-	      + _S_vtable[__t.index()](__edv::__get_storage(__t));
-	  }
-	return hash<size_t>{}(__t.index());
+	    using _Type = __remove_cvref_t<decltype(__t_mem)>;
+	    if constexpr (!is_same_v<_Type,
+			             __detail::__variant::__variant_cookie>)
+	      __ret = std::hash<size_t>{}(__t.index())
+		      + std::hash<_Type>{}(__t_mem);
+	    else
+	      __ret = std::hash<size_t>{}(__t.index());
+	    return {};
+	  }, __t);
+	return __ret;
       }
     };
 
diff --git a/libstdc++-v3/testsuite/20_util/variant/compile.cc b/libstdc++-v3/testsuite/20_util/variant/compile.cc
index 8a43092..04fef0b 100644
--- a/libstdc++-v3/testsuite/20_util/variant/compile.cc
+++ b/libstdc++-v3/testsuite/20_util/variant/compile.cc
@@ -488,12 +488,12 @@ void test_triviality()
   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, =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,  true,  true)
-  TEST_TEMPLATE(=default,         ,         , =default,         , false, false,  true, false)
+  TEST_TEMPLATE(=default,         ,         , =default, =default, false, false,  false,  true)
+  TEST_TEMPLATE(=default,         ,         , =default,         , false, false,  false, 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)
diff --git a/libstdc++-v3/testsuite/20_util/variant/run.cc b/libstdc++-v3/testsuite/20_util/variant/run.cc
index c5ea7df..7ee9b08 100644
--- a/libstdc++-v3/testsuite/20_util/variant/run.cc
+++ b/libstdc++-v3/testsuite/20_util/variant/run.cc
@@ -88,6 +88,21 @@ void arbitrary_ctor()
   VERIFY(get<1>(v) == "a");
 }
 
+struct ThrowingMoveCtorThrowsCopyCtor
+{
+  ThrowingMoveCtorThrowsCopyCtor() noexcept = default;
+  ThrowingMoveCtorThrowsCopyCtor(ThrowingMoveCtorThrowsCopyCtor&&) {}
+  ThrowingMoveCtorThrowsCopyCtor(ThrowingMoveCtorThrowsCopyCtor const&)
+  {
+    throw 0;
+  }
+
+  ThrowingMoveCtorThrowsCopyCtor& operator=(ThrowingMoveCtorThrowsCopyCtor&&) noexcept
+    = default;
+  ThrowingMoveCtorThrowsCopyCtor& operator=(ThrowingMoveCtorThrowsCopyCtor const&) noexcept
+    = default;
+};
+
 void copy_assign()
 {
   variant<monostate, string> v("a");
@@ -96,6 +111,20 @@ void copy_assign()
   u = v;
   VERIFY(holds_alternative<string>(u));
   VERIFY(get<string>(u) == "a");
+  {
+    std::variant<int, ThrowingMoveCtorThrowsCopyCtor> v1,
+      v2 = ThrowingMoveCtorThrowsCopyCtor();
+    bool should_throw = false;
+    try
+      {
+	v1 = v2;
+      }
+    catch(int)
+      {
+	should_throw = true;
+      }
+    VERIFY(should_throw);
+  }
 }
 
 void move_assign()
@@ -183,11 +212,15 @@ void emplace()
     AlwaysThrow a;
     try { v.emplace<1>(a); } catch (nullptr_t) { }
     VERIFY(v.valueless_by_exception());
+    v.emplace<0>(42);
+    VERIFY(!v.valueless_by_exception());
   }
   {
     variant<int, AlwaysThrow> v;
     try { v.emplace<1>(AlwaysThrow{}); } catch (nullptr_t) { }
     VERIFY(v.valueless_by_exception());
+    v.emplace<0>(42);
+    VERIFY(!v.valueless_by_exception());
   }
   VERIFY(&v.emplace<0>(1) == &std::get<0>(v));
   VERIFY(&v.emplace<int>(1) == &std::get<int>(v));
@@ -258,6 +291,7 @@ void test_relational()
     VERIFY(v < w);
     VERIFY(v <= w);
     VERIFY(!(v == w));
+    VERIFY(v == v);
     VERIFY(v != w);
     VERIFY(w > v);
     VERIFY(w >= v);
Jonathan Wakely March 6, 2019, 10:08 a.m. | #11
On 06/03/19 11:56 +0200, Ville Voutilainen wrote:
>On Wed, 6 Mar 2019 at 11:33, Jonathan Wakely <jwakely@redhat.com> wrote:

>> >+      else if constexpr (is_rvalue_reference_v<_Tp&&>)

>>

>> I know what this is doing, but it still looks a little odd to ask if

>> T&& is an rvalue-reference.

>>

>> Would it be clearer to structure this as:

>>

>>       if constexpr (is_lvalue_reference_v<_Tp>)

>>         {

>>           if constexpr (is_const_v<remove_reference_t<_Tp>>)

>>             return static_cast<const variant<_Types...>&>(__rhs);

>>           else

>>             return static_cast<variant<_Types...>&>(__rhs);

>>         }

>>       else

>>         return static_cast<variant<_Types...>&&>(__rhs);

>>

>> ?

>> >+          ::new (std::addressof(__this_mem))

>>

>> Is there any way that this can find the wrong operator new?

>>

>> Even if it can't, saying ::new ((void*)std::addressof(__this_mem))

>> would avoid having to think about that question again in future.

>>

>> Therre are a few other new expressions where that applies too (several

>> of them already there before your patch).

>> >+      ::new (&__storage) remove_reference_t<decltype(__storage)>

>>

>> This one definitely needs to be cast to void* and needs to use

>> addressof (or __addressof), otherwise ...

>

>

>Sure thing; an incremental diff attached.


>diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant

>index 5138599..4d81ceb 100644

>--- a/libstdc++-v3/include/std/variant

>+++ b/libstdc++-v3/include/std/variant

>@@ -145,12 +145,15 @@ namespace __variant

>   template <typename... _Types, typename _Tp>

>     decltype(auto) __variant_cast(_Tp&& __rhs)

>     {

>-      if constexpr (is_const_v<remove_reference_t<_Tp>>)

>-        return static_cast<const variant<_Types...>&>(__rhs);

>-      else if constexpr (is_rvalue_reference_v<_Tp&&>)

>-        return static_cast<variant<_Types...>&&>(__rhs);

>+      if constexpr (is_lvalue_reference_v<_Tp>)


As you mentioned on IRC, this also seems a bit odd ("why would it be
an lvalue reference?"), but such is the way of forwarding references
... they're not very intuitable. I think I have a weak preference for
doing it this way, so thanks for the change.

>+	{

>+	  if constexpr (is_const_v<remove_reference_t<_Tp>>)

>+			 return static_cast<const variant<_Types...>&>(__rhs);


Too many TABs here.

>+	  else

>+	    return static_cast<variant<_Types...>&>(__rhs);

>+	}

>       else

>-        return static_cast<variant<_Types...>&>(__rhs);

>+        return static_cast<variant<_Types...>&&>(__rhs);

>     }

> 

> namespace __detail

>@@ -212,7 +215,8 @@ namespace __variant

>     {

>       template<typename... _Args>

>       constexpr _Uninitialized(in_place_index_t<0>, _Args&&... __args)

>-      { ::new (&_M_storage) _Type(std::forward<_Args>(__args)...); }

>+      { ::new ((void*)std::addressof(_M_storage))

>+	  _Type(std::forward<_Args>(__args)...); }


Now that it doesn't fit in a single line, please put the braces on
separate lines:

      {
        ::new ((void*)std::addressof(_M_storage))
	  _Type(std::forward<_Args>(__args)...);
      }

Otherwise looks great, OK for trunk with those whitespace tweaks.
Thanks for doing this!

Patch

diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 89deb14..c2f82a7 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -138,6 +138,19 @@  namespace __variant
     constexpr variant_alternative_t<_Np, variant<_Types...>> const&&
     get(const variant<_Types...>&&);
 
+  template<typename _Visitor, typename... _Variants>
+    constexpr decltype(auto)
+    __do_visit(_Visitor&& __visitor, _Variants&&... __variants);
+
+  template <typename... _Types, typename _Tp>
+    decltype(auto) __variant_cast(_Tp&& __rhs)
+    {
+      if constexpr (is_const_v<remove_reference_t<_Tp>>)
+        return static_cast<const variant<_Types...>&>(__rhs);
+      else
+        return static_cast<variant<_Types...>&>(__rhs);
+    }
+
 namespace __detail
 {
 namespace __variant
@@ -155,6 +168,9 @@  namespace __variant
       std::integral_constant<size_t, is_same_v<_Tp, _First>
 	? 0 : __index_of_v<_Tp, _Rest...> + 1> {};
 
+  // used for raw visitation
+  struct __variant_cookie {};
+
   // _Uninitialized<T> is guaranteed to be a literal type, even if T is not.
   // We have to do this, because [basic.types]p10.5.3 (n4606) is not implemented
   // yet. When it's implemented, _Uninitialized<T> can be changed to the alias
@@ -246,11 +262,6 @@  namespace __variant
       ::new (__lhs) _Type(__variant::__ref_cast<_Rhs>(__rhs));
     }
 
-  template<typename _Variant, size_t _Np>
-    void
-    __erased_dtor(_Variant&& __v)
-    { std::_Destroy(std::__addressof(__variant::__get<_Np>(__v))); }
-
   template<typename _Lhs, typename _Rhs>
     void
     __erased_assign(void* __lhs, void* __rhs)
@@ -369,9 +380,6 @@  namespace __variant
   template<typename... _Types>
     struct _Variant_storage<false, _Types...>
     {
-      template<size_t... __indices>
-	static constexpr void (*_S_vtable[])(const _Variant_storage&) =
-	    { &__erased_dtor<const _Variant_storage&, __indices>... };
 
       constexpr _Variant_storage() : _M_index(variant_npos) { }
 
@@ -381,16 +389,21 @@  namespace __variant
 	_M_index(_Np)
 	{ }
 
-      template<size_t... __indices>
-	constexpr void _M_reset_impl(std::index_sequence<__indices...>)
-	{
-	  if (_M_index != __index_type(variant_npos))
-	    _S_vtable<__indices...>[_M_index](*this);
+      constexpr void _M_reset_impl()
+      {
+	__do_visit([](auto&& __this_mem) mutable
+		   -> __detail::__variant::__variant_cookie
+	  {
+	    if constexpr (!is_same_v<remove_reference_t<decltype(__this_mem)>,
+			  __variant_cookie>)
+	      std::_Destroy(std::__addressof(__this_mem));
+	    return {};
+	  }, __variant_cast<_Types...>(*this));
 	}
 
       void _M_reset()
       {
-	_M_reset_impl(std::index_sequence_for<_Types...>{});
+	_M_reset_impl();
 	_M_index = variant_npos;
       }
 
@@ -522,6 +535,20 @@  namespace __variant
 	  }
       }
 
+      void _M_destructive_copy(const _Move_ctor_base& __rhs)
+      {
+	this->~_Move_ctor_base();
+	__try
+	  {
+	    ::new (this) _Move_ctor_base(__rhs);
+	  }
+	__catch (...)
+	  {
+	    this->_M_index = variant_npos;
+	    __throw_exception_again;
+	  }
+      }
+
       _Move_ctor_base(const _Move_ctor_base&) = default;
       _Move_ctor_base& operator=(const _Move_ctor_base&) = default;
       _Move_ctor_base& operator=(_Move_ctor_base&&) = default;
@@ -538,6 +565,11 @@  namespace __variant
 	this->~_Move_ctor_base();
 	::new (this) _Move_ctor_base(std::move(__rhs));
       }
+      void _M_destructive_copy(const _Move_ctor_base& __rhs)
+      {
+	this->~_Move_ctor_base();
+	::new (this) _Move_ctor_base(__rhs);
+      }
     };
 
   template<typename... _Types>
@@ -554,21 +586,38 @@  namespace __variant
       operator=(const _Copy_assign_base& __rhs)
 	  noexcept(_Traits<_Types...>::_S_nothrow_copy_assign)
       {
-	if (this->_M_index == __rhs._M_index)
+	__do_visit([this, &__rhs](auto&& __this_mem, auto&& __rhs_mem) mutable
+		   -> __detail::__variant::__variant_cookie
 	  {
-	    if (__rhs._M_valid())
+	    if constexpr (is_same_v<remove_reference_t<decltype(__this_mem)>, remove_reference_t<decltype(__rhs_mem)>>)
 	      {
-		static constexpr void (*_S_vtable[])(void*, void*) =
-		  { &__erased_assign<_Types&, const _Types&>... };
-		_S_vtable[__rhs._M_index](this->_M_storage(),
-					  __rhs._M_storage());
+		if constexpr (!is_same_v<remove_reference_t<decltype(__rhs_mem)>, __variant_cookie>)
+		  __this_mem = __rhs_mem;
 	      }
-	  }
-	else
-	  {
-	    _Copy_assign_base __tmp(__rhs);
-	    this->_M_destructive_move(std::move(__tmp));
-	  }
+	    else
+	      {
+		if constexpr (!is_same_v<remove_reference_t<decltype(__rhs_mem)>, __variant_cookie>)
+		  {
+		    using __rhs_type =
+		      remove_reference_t<decltype(__rhs_mem)>;
+		    if constexpr (is_nothrow_copy_constructible_v<__rhs_type>
+		      || !is_nothrow_move_constructible_v<__rhs_type>)
+		      {
+			this->_M_destructive_copy(__rhs);
+		      }
+		    else
+		      {
+			_Copy_assign_base __tmp(__rhs);
+			this->_M_destructive_move(std::move(__tmp));
+		      }
+		  }
+		else
+		  {
+		    this->_M_reset();
+		  }
+	      }
+	  return {};
+	}, __variant_cast<_Types...>(*this), __variant_cast<_Types...>(__rhs));
 	__glibcxx_assert(this->_M_index == __rhs._M_index);
 	return *this;
       }
@@ -587,7 +636,8 @@  namespace __variant
 
   template<typename... _Types>
     using _Copy_assign_alias =
-	_Copy_assign_base<_Traits<_Types...>::_S_trivial_copy_assign,
+	_Copy_assign_base<_Traits<_Types...>::_S_trivial_copy_assign
+			  && _Traits<_Types...>::_S_trivial_copy_ctor,
 			  _Types...>;
 
   template<bool, typename... _Types>
@@ -733,15 +783,21 @@  namespace __variant
       _Tp _M_data;
     };
 
-  template<typename _Tp, size_t __first, size_t... __rest>
-    struct _Multi_array<_Tp, __first, __rest...>
+  template<typename _Ret,
+	   typename _Visitor,
+	   typename... _Variants,
+	   size_t __first, size_t... __rest>
+    struct _Multi_array<_Ret(*)(_Visitor, _Variants...), __first, __rest...>
     {
+      static constexpr int __do_cookie =
+	is_same_v<_Ret, __variant_cookie> ? 1 : 0;
+      using _Tp = _Ret(*)(_Visitor, _Variants...);
       template<typename... _Args>
 	constexpr const _Tp&
 	_M_access(size_t __first_index, _Args... __rest_indices) const
-	{ return _M_arr[__first_index]._M_access(__rest_indices...); }
+        { return _M_arr[__first_index + __do_cookie]._M_access(__rest_indices...); }
 
-      _Multi_array<_Tp, __rest...> _M_arr[__first];
+      _Multi_array<_Tp, __rest...> _M_arr[__first + __do_cookie];
     };
 
   // Creates a multi-dimensional vtable recursively.
@@ -801,18 +857,37 @@  namespace __variant
 	_S_apply_all_alts(_Array_type& __vtable,
 			  std::index_sequence<__var_indices...>)
 	{
-	  (_S_apply_single_alt<__var_indices>(
-	     __vtable._M_arr[__var_indices]), ...);
+	  if constexpr (is_same_v<_Result_type, __variant_cookie>)
+	    (_S_apply_single_alt<true, __var_indices>(
+	      __vtable._M_arr[__var_indices + 1],
+	      &(__vtable._M_arr[0])), ...);
+	  else
+	    (_S_apply_single_alt<false, __var_indices>(
+	      __vtable._M_arr[__var_indices]), ...);
 	}
 
-      template<size_t __index, typename _Tp>
+      template<bool __do_cookie, size_t __index, typename _Tp>
 	static constexpr void
-	_S_apply_single_alt(_Tp& __element)
+	_S_apply_single_alt(_Tp& __element, _Tp* __cookie_element = nullptr)
 	{
 	  using _Alternative = variant_alternative_t<__index, _Next>;
-	  __element = __gen_vtable_impl<
-	    remove_reference_t<decltype(__element)>, tuple<_Variants...>,
-	    std::index_sequence<__indices..., __index>>::_S_apply();
+	  if constexpr (__do_cookie)
+	    {
+	      __element = __gen_vtable_impl<
+		_Tp,
+		tuple<_Variants...>,
+		std::index_sequence<__indices..., __index>>::_S_apply();
+	      *__cookie_element = __gen_vtable_impl<
+		_Tp,
+		tuple<_Variants...>,
+		std::index_sequence<__indices..., variant_npos>>::_S_apply();
+	    }
+	  else
+	    {
+	      __element = __gen_vtable_impl<
+		remove_reference_t<decltype(__element)>, tuple<_Variants...>,
+		std::index_sequence<__indices..., __index>>::_S_apply();
+	    }
 	}
     };
 
@@ -825,11 +900,22 @@  namespace __variant
       using _Array_type =
 	  _Multi_array<_Result_type (*)(_Visitor&&, _Variants...)>;
 
+      template<size_t __index, typename _Variant>
+	static constexpr decltype(auto)
+	__element_by_index_or_cookie(_Variant&& __var)
+        {
+	  if constexpr (__index != variant_npos)
+	    return __variant::__get<__index>(std::forward<_Variant>(__var));
+	  else
+	    return __variant_cookie{};
+	}
+
       static constexpr decltype(auto)
       __visit_invoke(_Visitor&& __visitor, _Variants... __vars)
       {
 	return std::__invoke(std::forward<_Visitor>(__visitor),
-	    __variant::__get<__indices>(std::forward<_Variants>(__vars))...);
+	    __element_by_index_or_cookie<__indices>(
+	      std::forward<_Variants>(__vars))...);
       }
 
       static constexpr auto
@@ -843,7 +929,9 @@  namespace __variant
       using _Func_ptr = _Result_type (*)(_Visitor&&, _Variants...);
       using _Array_type =
 	  _Multi_array<_Func_ptr,
-		       variant_size_v<remove_reference_t<_Variants>>...>;
+		       (variant_size_v<remove_reference_t<_Variants>>
+			+ (is_same_v<_Result_type, __variant_cookie> ? 1 : 0))
+	               ...>;
 
       static constexpr _Array_type
       _S_apply()
@@ -1036,6 +1124,9 @@  namespace __variant
 	variant<_Types...>>
     {
     private:
+      template <typename... _UTypes, typename _Tp>
+	friend decltype(auto) __variant_cast(_Tp&&);
+
       static_assert(sizeof...(_Types) > 0,
 		    "variant must have at least one alternative");
       static_assert(!(std::is_reference_v<_Types> || ...),
@@ -1185,7 +1276,6 @@  namespace __variant
 	{
 	  static_assert(_Np < sizeof...(_Types),
 			"The index should be in [0, number of alternatives)");
-
 	  using type = variant_alternative_t<_Np, variant>;
 	  // If constructing the value can throw but move assigning it can't,
 	  // construct in a temporary and then move assign from it. This gives
@@ -1202,7 +1292,7 @@  namespace __variant
 	      return std::get<_Np>(*this);
 	    }
 
-	  this->~variant();
+	  this->_M_reset();
 	  __try
 	    {
 	      ::new (this) variant(in_place_index<_Np>,
@@ -1225,7 +1315,6 @@  namespace __variant
 	{
 	  static_assert(_Np < sizeof...(_Types),
 			"The index should be in [0, number of alternatives)");
-
 	  using type = variant_alternative_t<_Np, variant>;
 	  if constexpr (is_trivially_copyable_v<type>
 	      && !is_nothrow_constructible_v<type, initializer_list<_Up>,
@@ -1239,7 +1328,7 @@  namespace __variant
 	      return std::get<_Np>(*this);
 	    }
 
-	  this->~variant();
+	  this->_M_reset();
 	  __try
 	    {
 	      ::new (this) variant(in_place_index<_Np>, __il,
@@ -1401,11 +1490,8 @@  namespace __variant
 
   template<typename _Visitor, typename... _Variants>
     constexpr decltype(auto)
-    visit(_Visitor&& __visitor, _Variants&&... __variants)
+    __do_visit(_Visitor&& __visitor, _Variants&&... __variants)
     {
-      if ((__variants.valueless_by_exception() || ...))
-	__throw_bad_variant_access("Unexpected index");
-
       using _Result_type =
 	decltype(std::forward<_Visitor>(__visitor)(
 	    std::get<0>(std::forward<_Variants>(__variants))...));
@@ -1418,6 +1504,17 @@  namespace __variant
 			   std::forward<_Variants>(__variants)...);
     }
 
+  template<typename _Visitor, typename... _Variants>
+    constexpr decltype(auto)
+    visit(_Visitor&& __visitor, _Variants&&... __variants)
+    {
+      if ((__variants.valueless_by_exception() || ...))
+	__throw_bad_variant_access("Unexpected index");
+
+      return __do_visit(std::forward<_Visitor>(__visitor),
+			std::forward<_Variants>(__variants)...);
+    }
+
   template<bool, typename... _Types>
     struct __variant_hash_call_base_impl
     {
diff --git a/libstdc++-v3/testsuite/20_util/variant/compile.cc b/libstdc++-v3/testsuite/20_util/variant/compile.cc
index 8a43092..04fef0b 100644
--- a/libstdc++-v3/testsuite/20_util/variant/compile.cc
+++ b/libstdc++-v3/testsuite/20_util/variant/compile.cc
@@ -488,12 +488,12 @@  void test_triviality()
   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, =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,  true,  true)
-  TEST_TEMPLATE(=default,         ,         , =default,         , false, false,  true, false)
+  TEST_TEMPLATE(=default,         ,         , =default, =default, false, false,  false,  true)
+  TEST_TEMPLATE(=default,         ,         , =default,         , false, false,  false, 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)
diff --git a/libstdc++-v3/testsuite/20_util/variant/run.cc b/libstdc++-v3/testsuite/20_util/variant/run.cc
index c5ea7df..565b0e2 100644
--- a/libstdc++-v3/testsuite/20_util/variant/run.cc
+++ b/libstdc++-v3/testsuite/20_util/variant/run.cc
@@ -88,6 +88,21 @@  void arbitrary_ctor()
   VERIFY(get<1>(v) == "a");
 }
 
+struct ThrowingMoveCtorThrowsCopyCtor
+{
+  ThrowingMoveCtorThrowsCopyCtor() noexcept = default;
+  ThrowingMoveCtorThrowsCopyCtor(ThrowingMoveCtorThrowsCopyCtor&&) {}
+  ThrowingMoveCtorThrowsCopyCtor(ThrowingMoveCtorThrowsCopyCtor const&)
+  {
+    throw 0;
+  }
+
+  ThrowingMoveCtorThrowsCopyCtor& operator=(ThrowingMoveCtorThrowsCopyCtor&&) noexcept
+    = default;
+  ThrowingMoveCtorThrowsCopyCtor& operator=(ThrowingMoveCtorThrowsCopyCtor const&) noexcept
+    = default;
+};
+
 void copy_assign()
 {
   variant<monostate, string> v("a");
@@ -96,6 +111,20 @@  void copy_assign()
   u = v;
   VERIFY(holds_alternative<string>(u));
   VERIFY(get<string>(u) == "a");
+  {
+    std::variant<int, ThrowingMoveCtorThrowsCopyCtor> v1,
+      v2 = ThrowingMoveCtorThrowsCopyCtor();
+    bool should_throw = false;
+    try
+      {
+	v1 = v2;
+      }
+    catch(int)
+      {
+	should_throw = true;
+      }
+    VERIFY(should_throw);
+  }
 }
 
 void move_assign()
@@ -183,11 +212,15 @@  void emplace()
     AlwaysThrow a;
     try { v.emplace<1>(a); } catch (nullptr_t) { }
     VERIFY(v.valueless_by_exception());
+    v.emplace<0>(42);
+    VERIFY(!v.valueless_by_exception());
   }
   {
     variant<int, AlwaysThrow> v;
     try { v.emplace<1>(AlwaysThrow{}); } catch (nullptr_t) { }
     VERIFY(v.valueless_by_exception());
+    v.emplace<0>(42);
+    VERIFY(!v.valueless_by_exception());
   }
   VERIFY(&v.emplace<0>(1) == &std::get<0>(v));
   VERIFY(&v.emplace<int>(1) == &std::get<int>(v));