Review Hashtable extract node API

François Dumont frs.dumont@gmail.com
Thu Jun 20 20:40:00 GMT 2019


On 6/19/19 12:47 AM, Jonathan Wakely wrote:
> On 18/06/19 22:42 +0200, François Dumont wrote:
>> 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.
>
> Ha! I see, thanks. So I should have removed that key_type parameter
> again after removing the NodeAccessor stuff.
>
>
>> Ok to commit ?
>
> No, because that would restore the original signature of the
> _M_insert_unique_node function, but it has changed contract. Old
> callers who expect that function to delete the node would now leak
> memory if an exception is thrown.
>
Oh, yes, abi, I tend to forget even if the recent PR 90920 remind me 
about that, sorry.
> If we change the contract of the function we need to change its
> mangled name, so that callers expecting the old contract will not use
> the new function.
>
> I'll think about the best way to do that ...
>
Something like what's attached ?

I still use _GLIBCXX_INLINE_VERSION to tag functions that are kept just 
for abi-compatibility.

Ok to commit after having run tests ?

François

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


More information about the Libstdc++ mailing list