[PATCH] PR libstdc++/92124 on hashtable

[François Dumont]

On 1/6/20 4:17 PM, Jonathan Wakely wrote:
> 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?

The simpler the better. So here is the cleaned patch.

Regarding the comment, I also rewrite it a little cause copy/move of 
elements doesn't depend on _NodeGenerator anymore.

 Â Â Â  PR libstdc++/92124
 Â Â Â  * include/bits/hashtable.h (_Hashtable<>::__alloc_node_gen_t): New
 Â Â Â  template alias.
 Â Â Â  (_Hashtable<>::__fwd_value_for): 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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: hashtable_92124.patch
Type: text/x-patch
Size: 8986 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/libstdc++/attachments/20200108/28097ea6/attachment.bin>