Review Hashtable extract node API

[Jonathan Wakely]

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.

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 ...