[PATCH] PR libstdc++/92124 on hashtable

François Dumont frs.dumont@gmail.com
Wed Jan 8 05:43:00 GMT 2020


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>


More information about the Libstdc++ mailing list