[patch] Simplify allocator use
A couple of related patches that came out of some work on std::list and std::string which is not ready yet. One adds an alias template to simplify rebinding allocators. The other adds an RAII type to help manage pointers obtained from allocators. The new type means I can remove several ugly try-catch blocks that are all very similar in structure and have been bothering me for some time. The new type also makes it trivial to support allocators with fancy pointers, fixing long-standing (but not very important) bugs in std::promise and std::shared_ptr. Tested x86_64-linux, committed to trunk. commit 3309b9fc32a266f5bf296aec8e8fcbbe6f41d81b Author: Jonathan Wakely Date: Wed Jun 25 20:16:16 2014 +0100 * include/bits/alloc_traits.h (__alloc_rebind): Define alias template. * include/bits/forward_list.h (_Fwd_list_base): Use __alloc_rebind. * include/bits/hashtable_policy.h (_Insert_base, _Hashtable_alloc): Likewise. * include/ext/alloc_traits.h: Fix comment. diff --git a/libstdc++-v3/include/bits/alloc_traits.h b/libstdc++-v3/include/bits/alloc_traits.h index 23fe8de..3afcc6f 100644 --- a/libstdc++-v3/include/bits/alloc_traits.h +++ b/libstdc++-v3/include/bits/alloc_traits.h @@ -72,6 +72,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION typedef _Alloc<_Tp, _Args...> __type; }; + template +using __alloc_rebind = typename __alloctr_rebind<_Ptr, _Tp>::__type; + /** * @brief Uniform interface to all allocator types. * @ingroup allocators diff --git a/libstdc++-v3/include/bits/forward_list.h b/libstdc++-v3/include/bits/forward_list.h index 524fad1..7cf699e 100644 --- a/libstdc++-v3/include/bits/forward_list.h +++ b/libstdc++-v3/include/bits/forward_list.h @@ -274,13 +274,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER struct _Fwd_list_base { protected: - typedef typename __gnu_cxx::__alloc_traits<_Alloc> _Alloc_traits; - typedef typename _Alloc_traits::template rebind<_Tp>::other -_Tp_alloc_type; - - typedef typename _Alloc_traits::template -rebind<_Fwd_list_node<_Tp>>::other _Node_alloc_type; - + typedef __alloc_rebind<_Alloc, _Tp> _Tp_alloc_type; + typedef __alloc_rebind<_Alloc, _Fwd_list_node<_Tp>> _Node_alloc_type; typedef __gnu_cxx::__alloc_traits<_Node_alloc_type> _Node_alloc_traits; struct _Fwd_list_impl diff --git a/libstdc++-v3/include/bits/hashtable_policy.h b/libstdc++-v3/include/bits/hashtable_policy.h index ef15b0e..606fbab 100644 --- a/libstdc++-v3/include/bits/hashtable_policy.h +++ b/libstdc++-v3/include/bits/hashtable_policy.h @@ -701,8 +701,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION using __unique_keys = typename __hashtable_base::__unique_keys; using __ireturn_type = typename __hashtable_base::__ireturn_type; using __node_type = _Hash_node<_Value, _Traits::__hash_cached::value>; - using __node_alloc_type = - typename __alloctr_rebind<_Alloc, __node_type>::__type; + using __node_alloc_type = __alloc_rebind<_Alloc, __node_type>; using __node_gen_type = _AllocNode<__node_alloc_type>; __hashtable& @@ -1898,13 +1897,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION using __value_type = typename __node_type::value_type; using __value_alloc_type = - typename __alloctr_rebind<__node_alloc_type, __value_type>::__type; + __alloc_rebind<__node_alloc_type, __value_type>; using __value_alloc_traits = std::allocator_traits<__value_alloc_type>; using __node_base = __detail::_Hash_node_base; using __bucket_type = __node_base*; using __bucket_alloc_type = - typename __alloctr_rebind<__node_alloc_type, __bucket_type>::__type; + __alloc_rebind<__node_alloc_type, __bucket_type>; using __bucket_alloc_traits = std::allocator_traits<__bucket_alloc_type>; _Hashtable_alloc(const _Hashtable_alloc&) = default; diff --git a/libstdc++-v3/include/ext/alloc_traits.h b/libstdc++-v3/include/ext/alloc_traits.h index 14fbc43..abdb611 100644 --- a/libstdc++-v3/include/ext/alloc_traits.h +++ b/libstdc++-v3/include/ext/alloc_traits.h @@ -210,6 +210,6 @@ template }; _GLIBCXX_END_NAMESPACE_VERSION -} // namespace std +} // namespace __gnu_cxx #endif commit 6339d3559931228c0c3e4ce6e0cebcb0b0b0265d Author: Jonathan Wakely Date: Wed Jun 25 16:03:52 2014 +0100 * include/Makefile.am: Add new header. * include/Makefile.in: Regenerate. * include/bits/allocated_ptr.h (__allocated_ptr, __allocate_guarded): New RAII utilities for working with allocators. * include/bits/shared_ptr_base.h (_Sp_counted_deleter): Define __allocator_type typedef and use new __allocated_ptr type. (_Sp_counted_ptr_inplace): Likewise. (__shared_count::__shared_count, __shared_ptr::__shared_ptr): Use __allocate_guarded to to simplify exception handling. * include/experimental/any (any::_Manager_alloc::_S_alloc): Likewise. * include/std/future (_Result_alloc::_M_destroy): Likewise. (_Re
Re: [patch] Simplify allocator use
This simplifies some of the test changes in my last patch, I was misusing the CustomPointerAlloc due to confusion with some uncommitted changes. Tested x86_64-linux, committed to trunk. commit d1a05535e99bfecb427829d3e03ef82e0977e60c Author: Jonathan Wakely Date: Wed Jun 25 23:39:20 2014 +0100 * testsuite/20_util/shared_ptr/creation/alloc.cc: Fix use of test allocator. * testsuite/20_util/shared_ptr/creation/no_rtti.cc: Likewise. * testsuite/30_threads/promise/cons/alloc.cc: Likewise. diff --git a/libstdc++-v3/testsuite/20_util/shared_ptr/creation/alloc.cc b/libstdc++-v3/testsuite/20_util/shared_ptr/creation/alloc.cc index 402c612..0628807 100644 --- a/libstdc++-v3/testsuite/20_util/shared_ptr/creation/alloc.cc +++ b/libstdc++-v3/testsuite/20_util/shared_ptr/creation/alloc.cc @@ -101,16 +101,10 @@ test02() == tracker_allocator_counter::get_deallocation_count() ); } -template - struct Pointer : __gnu_test::PointerBase, T> - { -using __gnu_test::PointerBase, T>::PointerBase; - }; - void test03() { - __gnu_test::CustomPointerAlloc> alloc; + __gnu_test::CustomPointerAlloc alloc; auto p = std::allocate_shared(alloc, 1); VERIFY( *p == 1 ); } diff --git a/libstdc++-v3/testsuite/20_util/shared_ptr/creation/no_rtti.cc b/libstdc++-v3/testsuite/20_util/shared_ptr/creation/no_rtti.cc index 127bafb..d1fab6c 100644 --- a/libstdc++-v3/testsuite/20_util/shared_ptr/creation/no_rtti.cc +++ b/libstdc++-v3/testsuite/20_util/shared_ptr/creation/no_rtti.cc @@ -29,13 +29,7 @@ struct X { }; // test allocate_shared with no RTTI -template - struct Pointer : __gnu_test::PointerBase, T> - { -using __gnu_test::PointerBase, T>::PointerBase; - }; - -__gnu_test::CustomPointerAlloc> alloc; +__gnu_test::CustomPointerAlloc alloc; auto p = std::allocate_shared(alloc); diff --git a/libstdc++-v3/testsuite/30_threads/promise/cons/alloc.cc b/libstdc++-v3/testsuite/30_threads/promise/cons/alloc.cc index c45e646..ea9bb1a 100644 --- a/libstdc++-v3/testsuite/30_threads/promise/cons/alloc.cc +++ b/libstdc++-v3/testsuite/30_threads/promise/cons/alloc.cc @@ -38,16 +38,9 @@ void test01() VERIFY( p1.get_future().get() == 5 ); } -template - struct Pointer : __gnu_test::PointerBase, T> - { -using __gnu_test::PointerBase, T>::PointerBase; - }; - -void -test02() +void test02() { - __gnu_test::CustomPointerAlloc> alloc; + __gnu_test::CustomPointerAlloc alloc; promise p1(allocator_arg, alloc); p1.set_value(5); VERIFY( p1.get_future().get() == 5 );
Re: [patch] Simplify allocator use
On 25/06/14 21:56 +0100, Jonathan Wakely wrote: The other adds an RAII type to help manage pointers obtained from allocators. The new type means I can remove several ugly try-catch blocks that are all very similar in structure and have been bothering me for some time. The new type also makes it trivial to support allocators with fancy pointers, fixing long-standing (but not very important) bugs in std::promise and std::shared_ptr. This patch applies the __allocated_ptr type to hashtable_policy.h to remove most explicit deallocation (yay!) The buckets are still allocated and deallocated manually, because __allocated_ptr only works for allocations of single objects, not arrays. As well as __allocated_ptr this change relies on two things: 1) the node type has a trivial destructor, so we don't actually need to call it, we can just reuse or release its storage. (See 3.8 [basic.life] p1) 2) allocator_traits::construct and allocator_traits::destroy can be used with an allocator that has a different value_type, so we don't need to create a rebound copy to destroy every element, we can just use the node-allocator. (See http://cplusplus.github.io/LWG/lwg-active.html#2218 which is Open, but I've discussed the issue with Howard, Pablo and others, and I think libc++ already relies on this assumption). François, could you check it, and let me know if you see anything wrong or have any comments? commit d2fd02daab715c79c766bc0a476d1d01da1fc305 Author: Jonathan Wakely Date: Thu Jun 26 12:28:56 2014 +0100 * include/bits/hashtable_policy.h (_ReuseOrAllocNode::operator()): Use __allocated_ptr. (_Hashtable_alloc::_M_allocate_node): Likewise. (_Hashtable_alloc::_M_deallocate_node): Likewise. diff --git a/libstdc++-v3/include/bits/hashtable_policy.h b/libstdc++-v3/include/bits/hashtable_policy.h index 606fbab..ed6b2d7 100644 --- a/libstdc++-v3/include/bits/hashtable_policy.h +++ b/libstdc++-v3/include/bits/hashtable_policy.h @@ -31,6 +31,8 @@ #ifndef _HASHTABLE_POLICY_H #define _HASHTABLE_POLICY_H 1 +#include + namespace std _GLIBCXX_VISIBILITY(default) { _GLIBCXX_BEGIN_NAMESPACE_VERSION @@ -137,20 +139,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __node_type* __node = _M_nodes; _M_nodes = _M_nodes->_M_next(); __node->_M_nxt = nullptr; - __value_alloc_type __a(_M_h._M_node_allocator()); - __value_alloc_traits::destroy(__a, __node->_M_valptr()); - __try - { - __value_alloc_traits::construct(__a, __node->_M_valptr(), - std::forward<_Arg>(__arg)); - } - __catch(...) - { - __node->~__node_type(); - __node_alloc_traits::deallocate(_M_h._M_node_allocator(), - __node, 1); - __throw_exception_again; - } + auto& __a = _M_h._M_node_allocator(); + __node_alloc_traits::destroy(__a, __node->_M_valptr()); + __allocated_ptr<_NodeAlloc> __guard{__a, __node}; + __node_alloc_traits::construct(__a, __node->_M_valptr(), + std::forward<_Arg>(__arg)); + __guard = nullptr; return __node; } return _M_h._M_allocate_node(std::forward<_Arg>(__arg)); @@ -1947,33 +1941,25 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION typename _Hashtable_alloc<_NodeAlloc>::__node_type* _Hashtable_alloc<_NodeAlloc>::_M_allocate_node(_Args&&... __args) { - auto __nptr = __node_alloc_traits::allocate(_M_node_allocator(), 1); - __node_type* __n = std::__addressof(*__nptr); - __try - { - __value_alloc_type __a(_M_node_allocator()); - ::new ((void*)__n) __node_type; - __value_alloc_traits::construct(__a, __n->_M_valptr(), - std::forward<_Args>(__args)...); - return __n; - } - __catch(...) - { - __node_alloc_traits::deallocate(_M_node_allocator(), __nptr, 1); - __throw_exception_again; - } + auto& __a = _M_node_allocator(); + auto __guard = std::__allocate_guarded(__a); + __node_type* __n = __guard.get(); + ::new ((void*)__n) __node_type; + __node_alloc_traits::construct(__a, __n->_M_valptr(), + std::forward<_Args>(__args)...); + __guard = nullptr; + return __n; } template void _Hashtable_alloc<_NodeAlloc>::_M_deallocate_node(__node_type* __n) { - typedef typename __node_alloc_traits::pointer _Ptr; - auto __ptr = std::pointer_traits<_Ptr>::pointer_to(*__n); - __value_alloc_type __a(_M_node_allocator()); - __value_alloc_traits::destroy(__a, __n->_M_valptr()); - __n->~__node_type(); - __node_alloc_traits::deallocate(_M_node_allocator(), __ptr, 1); + static_assert(std::is_trivially_destructible<__node_type>::value, + "Nodes must not require non-trivial destruction"); + auto& __alloc = _M_node_allocator(); + __allocated_ptr<__node_alloc_type> __guard{__alloc, __n}; + __node_alloc_traits::destroy(__alloc, __n->_M_valptr()); } template
Re: [patch] Simplify allocator use
On 26/06/14 12:31 +0100, Jonathan Wakely wrote: @@ -137,20 +139,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __node_type* __node = _M_nodes; _M_nodes = _M_nodes->_M_next(); __node->_M_nxt = nullptr; - __value_alloc_type __a(_M_h._M_node_allocator()); - __value_alloc_traits::destroy(__a, __node->_M_valptr()); - __try - { - __value_alloc_traits::construct(__a, __node->_M_valptr(), - std::forward<_Arg>(__arg)); - } - __catch(...) - { - __node->~__node_type(); - __node_alloc_traits::deallocate(_M_h._M_node_allocator(), - __node, 1); I forgot to mention the change also fixes a bug in the line above: __node should be converted to the allocator's pointer type before passing it to deallocate. Using __allocated_ptr takes care of that.
Re: [patch] Simplify allocator use
On 26/06/14 00:06 +0100, Jonathan Wakely wrote: This simplifies some of the test changes in my last patch, I was misusing the CustomPointerAlloc due to confusion with some uncommitted changes. And this fixes the -fno-rtti version of make_shared, I shouldn't have changed the deleter's parameter to the allocator's pointer. That worked with the current test, but only because our CustomPointerAlloc uses a custom pointer that is implicitly-convertible from value_type*. I have a completely rewritten custom pointer for the testsuite which doesn't support implicit conversions (only the minimum requirements) and that caught this bug. The new custom pointer type is proving very useful while I'm making some std::list changes but isn't ready for prime-time yet. Tested x86_64-linux, committed to trunk. commit e69d8134edde691db7ea2567032229b210dd263d Author: Jonathan Wakely Date: Thu Jun 26 13:27:30 2014 +0100 * include/bits/shared_ptr_base.h (__shared_ptr::_Deleter): Fix parameter type. diff --git a/libstdc++-v3/include/bits/shared_ptr_base.h b/libstdc++-v3/include/bits/shared_ptr_base.h index 590a8d3..6f85ffa 100644 --- a/libstdc++-v3/include/bits/shared_ptr_base.h +++ b/libstdc++-v3/include/bits/shared_ptr_base.h @@ -1085,7 +1085,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION template struct _Deleter { - void operator()(typename _Alloc::pointer __ptr) + void operator()(_Tp* __ptr) { __allocated_ptr<_Alloc> __guard{ _M_alloc, __ptr }; allocator_traits<_Alloc>::destroy(_M_alloc, __guard.get());
Re: [patch] Simplify allocator use
On 26/06/2014 13:31, Jonathan Wakely wrote: On 25/06/14 21:56 +0100, Jonathan Wakely wrote: The other adds an RAII type to help manage pointers obtained from allocators. The new type means I can remove several ugly try-catch blocks that are all very similar in structure and have been bothering me for some time. The new type also makes it trivial to support allocators with fancy pointers, fixing long-standing (but not very important) bugs in std::promise and std::shared_ptr. This patch applies the __allocated_ptr type to hashtable_policy.h to remove most explicit deallocation (yay!) The buckets are still allocated and deallocated manually, because __allocated_ptr only works for allocations of single objects, not arrays. As well as __allocated_ptr this change relies on two things: 1) the node type has a trivial destructor, so we don't actually need to call it, we can just reuse or release its storage. (See 3.8 [basic.life] p1) 2) allocator_traits::construct and allocator_traits::destroy can be used with an allocator that has a different value_type, so we don't need to create a rebound copy to destroy every element, we can just use the node-allocator. (See http://cplusplus.github.io/LWG/lwg-active.html#2218 which is Open, but I've discussed the issue with Howard, Pablo and others, and I think libc++ already relies on this assumption). François, could you check it, and let me know if you see anything wrong or have any comments? That looks fine to me, nice simplification. François