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

[Jonathan Wakely]

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!