Review Hashtable extract node API
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
>>>> 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);
>>>> +Â Â Â =
>>>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â __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
Ok to commit after having run tests ?
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 8792 bytes
Desc: not available
More information about the Libstdc++