Re: Fix hashtable node deallocation

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: Fix hashtable node deallocation

François Dumont-2
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


On 11/10/18 10:40 PM, François Dumont wrote:

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


hashtable_dealloc.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Fix hashtable node deallocation

Jonathan Wakely-3
On 17/11/18 22:01 +0100, François Dumont wrote:

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

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

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

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


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