On 05/03/19 23:27 +0200, Ville Voutilainen wrote:
On Mon, 4 Mar 2019 at 01:43, Ville Voutilainen
<ville.voutilai...@gmail.com> wrote:

On Mon, 4 Mar 2019 at 01:26, Ville Voutilainen
<ville.voutilai...@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.voutilai...@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>();
}

Reply via email to