This is the mail archive of the libstdc++@gcc.gnu.org mailing list for the libstdc++ project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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!



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]