Fix hashtable node deallocation

[François Dumont]

While working on a hashtable enhancement I noticed that we are not using
the correct method to deallocate node if the constructor throws in
_ReuseOrAllocNode operator(). I had to introduce a new
_M_deallocate_node_ptr for that as node value shall not be destroy again.

I also check other places and noticed that a __node_type destructor call
was missing.

    * include/bits/hashtable_policy.h
(_Hashtable_alloc<>::_M_deallocate_node_ptr(__node_type*)): New.
    (_Hashtable_alloc<>::_M_deallocate_node(__node_type*)): Use latter.
(_ReuseOrAllocNode<>::operator<_Arg>()(_Arg&&)): Likewise.
    (_Hashtable_alloc<>::_M_allocate_node): Add ~__node_type call.

Tested under Linux x86_64.

Ok to commit ?

François

[François Dumont]

Here is the same patch but this time with a test change which is
supposed to show the problem.

However it doesn't because of:
      _Pointer_adapter(element_type* __arg = 0)
      { _Storage_policy::set(__arg); }

which is not explicit.

So is this patch really necessary ? If it isn't, is usage of
pointer_traits<>::pointer_to really necessary ?

Note that I also found a bug in the
__gnu_test::CustomPointerAlloc::allocate, the signature with hint is wrong.

    * include/ext/throw_allocator.h
    (annotate_base::insert(void*, size_t)): Use insert result to check for
    double insert attempt.
    (annotate_base::insert_construct(void*)): Likewise.
    (annotate_base::check_allocated(void*, size_t)): Return found iterator.
    (annotate_base::erase(void*, size_t)): Use latter method returned
    iterator.
    (annotate_base::check_constructed(void*, size_t)): Return found
iterator.
    (annotate_base::erase_construct(void*)): Use latter method returned
    iterator.
    * libstdc++-v3/testsuite/util/testsuite_allocator.h
    (CustomPointerAlloc<>::allocate(size_t, pointer)): Replace by...
    (CustomPointerAlloc<>::allocate(size_t, const_void_pointer)): ...this.
    * testsuite/23_containers/unordered_set/allocator/ext_ptr.cc: Add
    check.

François

Post by François Dumont
While working on a hashtable enhancement I noticed that we are not
using the correct method to deallocate node if the constructor throws
in _ReuseOrAllocNode operator(). I had to introduce a new
_M_deallocate_node_ptr for that as node value shall not be destroy again.
I also check other places and noticed that a __node_type destructor
call was missing.
    * include/bits/hashtable_policy.h
(_Hashtable_alloc<>::_M_deallocate_node_ptr(__node_type*)): New.
    (_Hashtable_alloc<>::_M_deallocate_node(__node_type*)): Use latter.
(_ReuseOrAllocNode<>::operator<_Arg>()(_Arg&&)): Likewise.
    (_Hashtable_alloc<>::_M_allocate_node): Add ~__node_type call.
Tested under Linux x86_64.
Ok to commit ?
François

[Jonathan Wakely]

Post by François Dumont
Here is the same patch but this time with a test change which is
supposed to show the problem.
      _Pointer_adapter(element_type* __arg = 0)
      { _Storage_policy::set(__arg); }
which is not explicit.
So is this patch really necessary ? If it isn't, is usage of
pointer_traits<>::pointer_to really necessary ?

Yes. Just because our _Pointer_adapter allows implicit conversions
from raw pointers doesn't mean all fancy pointers allow that.

Post by François Dumont
Note that I also found a bug in the
__gnu_test::CustomPointerAlloc::allocate, the signature with hint is wrong.

Yes, that's a bug, thanks.

Post by François Dumont
    * include/ext/throw_allocator.h
    (annotate_base::insert(void*, size_t)): Use insert result to check for
    double insert attempt.
    (annotate_base::insert_construct(void*)): Likewise.
    (annotate_base::check_allocated(void*, size_t)): Return found iterator.
    (annotate_base::erase(void*, size_t)): Use latter method returned
    iterator.
    (annotate_base::check_constructed(void*, size_t)): Return found
iterator.
    (annotate_base::erase_construct(void*)): Use latter method returned
    iterator.

This looks like the wrong ChangeLog.

Post by François Dumont
    * libstdc++-v3/testsuite/util/testsuite_allocator.h
    (CustomPointerAlloc<>::allocate(size_t, pointer)): Replace by...
    (CustomPointerAlloc<>::allocate(size_t, const_void_pointer)): ...this.
    * testsuite/23_containers/unordered_set/allocator/ext_ptr.cc: Add
    check.

[Jonathan Wakely]

Post by François Dumont
While working on a hashtable enhancement I noticed that we are not
using the correct method to deallocate node if the constructor throws
in _ReuseOrAllocNode operator(). I had to introduce a new
_M_deallocate_node_ptr for that as node value shall not be destroy again.
I also check other places and noticed that a __node_type destructor
call was missing.

That's intentional. The type has a trivial destructor, so its storage
can just be reused, we don't need to destroy it.

[François Dumont]

Post by Jonathan Wakely

Post by François Dumont
While working on a hashtable enhancement I noticed that we are not
using the correct method to deallocate node if the constructor throws
in _ReuseOrAllocNode operator(). I had to introduce a new
_M_deallocate_node_ptr for that as node value shall not be destroy again.
I also check other places and noticed that a __node_type destructor
call was missing.

That's intentional. The type has a trivial destructor, so its storage
can just be reused, we don't need to destroy it.

Ok, do you want to also remove the other call to ~__node_type() then ?

Here is the updated patch and the right ChangeLog entry:

    * include/bits/hashtable_policy.h
(_Hashtable_alloc<>::_M_deallocate_node_ptr(__node_type*)): New.
    (_Hashtable_alloc<>::_M_deallocate_node(__node_type*)): Use latter.
(_ReuseOrAllocNode<>::operator<_Arg>()(_Arg&&)): Likewise.
    (_Hashtable_alloc<>::_M_allocate_node): Add ~__node_type call.
    * libstdc++-v3/testsuite/util/testsuite_allocator.h
    (CustomPointerAlloc<>::allocate(size_t, pointer)): Replace by...
    (CustomPointerAlloc<>::allocate(size_t, const_void_pointer)): ...this.
    * testsuite/23_containers/unordered_set/allocator/ext_ptr.cc: Add
    check.

Ok to commit ?

François

[François Dumont]

I am unclear about this patch, is it accepted ?

Post by François Dumont

Post by Jonathan Wakely

Post by François Dumont
While working on a hashtable enhancement I noticed that we are not
using the correct method to deallocate node if the constructor
throws in _ReuseOrAllocNode operator(). I had to introduce a new
_M_deallocate_node_ptr for that as node value shall not be destroy again.
I also check other places and noticed that a __node_type destructor
call was missing.

That's intentional. The type has a trivial destructor, so its storage
can just be reused, we don't need to destroy it.

Ok, do you want to also remove the other call to ~__node_type() then ?
    * include/bits/hashtable_policy.h
(_Hashtable_alloc<>::_M_deallocate_node_ptr(__node_type*)): New.
    (_Hashtable_alloc<>::_M_deallocate_node(__node_type*)): Use latter.
(_ReuseOrAllocNode<>::operator<_Arg>()(_Arg&&)): Likewise.
    (_Hashtable_alloc<>::_M_allocate_node): Add ~__node_type call.
    * libstdc++-v3/testsuite/util/testsuite_allocator.h
    (CustomPointerAlloc<>::allocate(size_t, pointer)): Replace by...
...this.
    * testsuite/23_containers/unordered_set/allocator/ext_ptr.cc: Add
    check.
Ok to commit ?
François