This is the mail archive of the
libstdc++@gcc.gnu.org
mailing list for the libstdc++ project.
Re: [v3 PATCH, RFC] Rewrite variant. Also PR libstdc++/85517
- From: Jonathan Wakely <jwakely at redhat dot com>
- To: Ville Voutilainen <ville dot voutilainen at gmail dot com>
- Cc: libstdc++ <libstdc++ at gcc dot gnu dot org>, gcc-patches List <gcc-patches at gcc dot gnu dot org>
- Date: Wed, 6 Mar 2019 10:08:58 +0000
- Subject: Re: [v3 PATCH, RFC] Rewrite variant. Also PR libstdc++/85517
- References: <CAFk2RUaH6GRp_FqyHXPso_jy2WVdrc_QtdvWz=+9w5F-mBW93A@mail.gmail.com> <20190206101339.GA17805@redhat.com> <CAFk2RUbvq-DkELfhPuXLgqvHYiZN+t7uW23UW-pcY---+bpGYw@mail.gmail.com> <CAFk2RUa78j3DnEVFP3=a0fQghraBX6TZX+JNb4E3QwGd9GNnMQ@mail.gmail.com> <CAFk2RUZc24RTA77Bri9ekWNQrDNxHgg9S=u_WHw0GjeO5oivKA@mail.gmail.com> <CAFk2RUZNkuw0i8F0u4-WuKh8D2O9VWTRE+XzmordqNtX2x1UWQ@mail.gmail.com> <CAFk2RUa8rrJC7yenrM7Z47u9K2BdQfikvzxdnWMO3aysz9opVg@mail.gmail.com> <20190306093305.GF21503@redhat.com> <CAFk2RUa9426D9r7W5WMfrH8N1wCtZ3g3sE=EKjynuMKsrDdSDQ@mail.gmail.com>
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!