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

[Jonathan Wakely]

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>();
}