On 06/03/19 11:56 +0200, Ville Voutilainen wrote:
On Wed, 6 Mar 2019 at 11:33, Jonathan Wakely <jwak...@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!
