Review Hashtable extract node API

[François Dumont]

On 6/18/19 12:54 PM, Jonathan Wakely wrote:
> On 18/06/19 07:52 +0200, François Dumont wrote:
>> A small regression noticed while merging.
>>
>> We shouldn't keep on using a moved-from key_type instance.
>>
>> Ok to commit ? Feel free to do it if you prefer, I'll do so at end of 
>> Europe day otherwise.
>
>
>> diff --git a/libstdc++-v3/include/bits/hashtable_policy.h 
>> b/libstdc++-v3/include/bits/hashtable_policy.h
>> index f5809c7443a..7e89e1b44c4 100644
>> --- a/libstdc++-v3/include/bits/hashtable_policy.h
>> +++ b/libstdc++-v3/include/bits/hashtable_policy.h
>> @@ -743,7 +743,8 @@ namespace __detail
>> Â Â Â Â std::tuple<>()
>> Â Â Â Â Â  };
>> Â Â Â Â Â  auto __pos
>> -Â Â Â  = __h->_M_insert_unique_node(__k, __bkt, __code, __node._M_node);
>> +Â Â Â  = 
>> __h->_M_insert_unique_node(__h->_M_extract()(__node._M_node->_M_v()),
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  __bkt, __code, __node._M_node);
>> Â Â Â Â Â  __node._M_node = nullptr;
>> Â Â Â Â Â  return __pos->second;
>> Â Â Â  }
>
> I can't create an example where this causes a problem, because the key
> passed to _M_insert_unique_node is never used. So it doesn't matter
> that it's been moved from.
>
> So I have to wonder why we just added the key parameter to that
> function, if it's never used.

I think you've been influence by my patch. I was using a "_NodeAccessor" 
which wasn't giving access to the node without taking owership so I 
needed to pass the key properly to compute new bucket index in case of 
rehash.

But with your approach this change to the _M_insert_unique_node was 
simply unecessary so here is a patch to cleanup this part.

Ok to commit ?


>
> As far as I can tell, it would only be used for a non-default range
> hash function, and I don't care about that. Frankly I find the
> policy-based _Hashtable completely unmaintainable and I'd gladly get
> rid of all of it that isn't needed for the std::unordered_xxx
> containers. The non-standard extensions are not used by anybody,
> apparently not tested properly (or this regression should have been
> noticed) and make the code too complicated.
I never consider removing this but it would indeed make maintainance 
easy. I'll add to my TODO.
>
> We're adding new parameters that have to be passed around even though
> they're never used by 99.999% of users. No wonder the code is only
> fast at -O3.
>
> </rant>
>
>
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: hashtable.patch
Type: text/x-patch
Size: 3869 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/libstdc++/attachments/20190618/6bcd9942/attachment.bin>