[PATCH] PR libstdc++/92124 on hashtable

[Jonathan Wakely]

On 07/11/19 20:28 +0100, François Dumont wrote:
>From what I understood from recent fix the unordered containers need 
>to be updated the same way.
>
>I hope you'll appreciate the usage of rvalue forwarding. Containers 

Yes, I think it makes sense.

>node values are moved as soon as _M_assign is called with a rvalue 
>reference to the source container.
>
>Additionnaly this patch removes usages of lambdas in _Hashtable.
>
>If you confirm it I'll check for the same on _Rb_tree.
>
>    * include/bits/hashtable.h (_Hashtable<>::__alloc_node_gen_t): New
>    template alias.
>    (_Hashtable<>::__mv_if_value_type_mv_noexcept): New.
>    (_Hashtable<>::__fwd_value): New.
>    (_Hashtable<>::_M_assign_elements<>): Remove _NodeGenerator template
>    parameter.
>    (_Hashtable<>::_M_assign<>): Add _Ht template parameter.
>    (_Hashtable<>::operator=(const _Hashtable<>&)): Adapt.
>    (_Hashtable<>::_M_move_assign): Adapt.
>    (_Hashtable<>::_Hashtable(const _Hashtable&)): Adapt.
>    (_Hashtable<>::_Hashtable(const _Hashtable&, const allocator_type&)):
>    Adapt.
>    (_Hashtable<>::_Hashtable(_Hashtable&&, const allocator_type&)):
>    Adapt.
>    * testsuite/23_containers/unordered_set/92124.cc: New.
>
>Tested under Linux x86_64.
>
>Ok to commit ?
>
>François
>

>diff --git a/libstdc++-v3/include/bits/hashtable.h b/libstdc++-v3/include/bits/hashtable.h
>index ab579a7059e..c2b2219d471 100644
>--- a/libstdc++-v3/include/bits/hashtable.h
>+++ b/libstdc++-v3/include/bits/hashtable.h
>@@ -255,6 +255,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 
>       using __reuse_or_alloc_node_gen_t =
> 	__detail::_ReuseOrAllocNode<__node_alloc_type>;
>+      using __alloc_node_gen_t =
>+	__detail::_AllocNode<__node_alloc_type>;
> 
>       // Simple RAII type for managing a node containing an element
>       struct _Scoped_node
>@@ -280,6 +282,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 	__node_type* _M_node;
>       };
> 
>+      template<typename _Tp>
>+	static constexpr
>+	typename conditional<__move_if_noexcept_cond<value_type>::value,
>+			     const _Tp&, _Tp&&>::type
>+	__mv_if_value_type_mv_noexcept(_Tp& __x) noexcept
>+	{ return std::move(__x); }

This is only used in one place. Adding a function doesn't seem
worthwhile, you can just do this where you use it:

   using _Fwd_Ht = typename
     conditional<__move_if_noexcept_cond<value_type>::value,
                 const _Ht&, _Ht>::type;
   _M_assign(std::forward<_Fwd_Ht>(__ht), __alloc_gen);


>+      template<typename _Ht>
>+	static constexpr
>+	typename conditional<!std::is_lvalue_reference<_Ht>::value,
>+			     value_type&&, const value_type&>::type

I think I'd prefer to reverse the condition, i.e.

   typename conditional<is_lvalue_reference<_Ht>::value,
                        const value_type&, value_type&&>::type

>+	__fwd_value(_Ht&&, value_type& __val) noexcept
>+	{ return std::move(__val); }

Since this doesn't use the first parameter, it can just be removed:

   template<typename _Ht>
     static constexpr
     typename conditional<std::is_lvalue_reference<_Ht>::value,
                          const value_type&, value_type&&>::type
     __fwd_value(value_type& __val) noexcept
     { return std::move(__val); }

That simplifies the usage from:

   __fwd_value(std::forward<_Ht>(__ht), __ht_n->_M_v()))

so it becomes:

   __fwd_value<_Ht>(__ht_n->_M_v()))


Maybe __fwd_value<_Ht> should be __fwd_value_for<_Ht> so it's clear
how it depends on _Ht (because otherwise it looks like it is
forwarding as _Ht&& like std::forward<_Ht> would).

What do you think?