Re: Fix move_if_noexcept usages in _Hashtable
I reviewed this patch following result of making std::pair piecewise
constructor noexcept:
https://gcc.gnu.org/ml/libstdc++/2018-12/msg00052.html
I restore check of noexcept qualification on move constructor rather
than alloc::construct cause when dealing with std::pairValue> I want to check for noexcept qualification of Value type move
constructor and it doesn't really make sens to check for
alloc::construct when we eventually call allocValue>::construct.
I'll submit this officially once back in stage 1.
François
On 12/4/18 10:43 PM, François Dumont wrote:
On 12/4/18 3:38 PM, Jonathan Wakely wrote:
On 04/12/18 07:45 +0100, François Dumont wrote:
Hi
This patch fix a minor problem with usage of
std::move_if_noexcept. We use it to move node content if move
construtor is noexcept but we eventually use the
allocator_type::construct method which might be slightly different.
I think it is better to check for this method noexcept qualification.
This is likely to pessimize some code, since most allocators do not
have an exception-specification on their construct members.
Perhaps but the Standard mandates to call allocator construct so we
don't have choice. It is surprising to consider value_type move
constructor when we don't know what allocator construct does.
Most users do not change from std::allocator or, even if they do, do
not implement construct so impact should be limited.
Moreover I have added a special overload for nodes containing a
std::pair. It is supposed to allow move semantic in associative
containers where Key is stored as const deleting std::pair move
constructor. In this case we should still move the Value part.
It doesn't work for the moment because the std::pair piecewise
constructor has no noexcept qualification. Is there any plan to add
it ? I think adding it will force including in stl_pair.h,
is it fine ?
No feedback on this point ? Is using std::pair piecewise constructor ok ?
If this evolution is accepted I'll adapt it for _Rb_tree that has
the same problem.
Working on this I also notice that content of initialization_list
is not moved. Is there a plan to make initialization_list iterator
type like move_iterator ? Should containers use
__make_move_iterator_if_noexcept ?
No.
Whether to allow moving from std::initializer_list is an active topic,
see
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1249r0.html
Ok, nice, allowing emplace build of values would be even better, I'll
have a closer look.
Tested under Linux x86_64 normal mode.
Ok to commit this first step ?
No, this is not suitable for stage 3. It seems too risky.
We can reconsider it during stage 1, but I'd like to see (at least) a
new test showing a bug with the current code. For example, a type with
a move constructor that is noexcept, but when used with a
scoped_allocator_adaptor (which calls something other than the move
constructor) we incorrectly move elements, and lose data when an
exception happens.
Ok, I'll try.
diff --git a/libstdc++-v3/include/bits/hashtable.h b/libstdc++-v3/include/bits/hashtable.h
index 9a12ad399dc..dfb756cec07 100644
--- a/libstdc++-v3/include/bits/hashtable.h
+++ b/libstdc++-v3/include/bits/hashtable.h
@@ -266,8 +266,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_Equal, _H1, _H2, _Hash,
_RehashPolicy, _Traits>;
- using __reuse_or_alloc_node_type =
- __detail::_ReuseOrAllocNode<__node_alloc_type>;
+ template
+ using __alloc_node_gen_t =
+ __detail::_AllocNode<__node_alloc_type, _ConstructF>;
+
+ template
+ using __reuse_or_alloc_node_gen_t =
+ __detail::_ReuseOrAllocNode<__node_alloc_type, _ConstructF>;
// Metaprogramming for picking apart hash caching.
template
@@ -400,9 +405,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
// Assign *this using another _Hashtable instance. Either elements
// are copy or move depends on the _NodeGenerator.
- template
+ template
void
- _M_assign_elements(_Ht&&, const _NodeGenerator&);
+ _M_assign_elements(_Ht&&);
template
void
@@ -500,7 +505,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_Hashtable&
operator=(initializer_list __l)
{
- __reuse_or_alloc_node_type __roan(_M_begin(), *this);
+ __reuse_or_alloc_node_gen_t<_Construct_object_a>
+ __roan(*this, _M_begin());
_M_before_begin._M_nxt = nullptr;
clear();
this->_M_insert_range(__l.begin(), __l.end(), __roan, __unique_keys());
@@ -1047,9 +1053,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_M_rehash_policy = __ht._M_rehash_policy;
__try
{
- _M_assign(__ht,
- [this](const __node_type* __n)
- { return this->_M_allocate_node(__n->_M_v()); });
+ __alloc_node_gen_t<_Construct_object_a> __ang(*this);
+ _M_assign(__ht, __ang);
}
__catch(...)
{
@@ -1064,9 +1069,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
}
// Reuse allocated buckets and nodes.
- _M_assign_elements(__ht,
- [](const
Re: Fix move_if_noexcept usages in _Hashtable
On 12/4/18 3:38 PM, Jonathan Wakely wrote: On 04/12/18 07:45 +0100, François Dumont wrote: Hi This patch fix a minor problem with usage of std::move_if_noexcept. We use it to move node content if move construtor is noexcept but we eventually use the allocator_type::construct method which might be slightly different. I think it is better to check for this method noexcept qualification. This is likely to pessimize some code, since most allocators do not have an exception-specification on their construct members. Perhaps but the Standard mandates to call allocator construct so we don't have choice. It is surprising to consider value_type move constructor when we don't know what allocator construct does. Most users do not change from std::allocator or, even if they do, do not implement construct so impact should be limited. Moreover I have added a special overload for nodes containing a std::pair. It is supposed to allow move semantic in associative containers where Key is stored as const deleting std::pair move constructor. In this case we should still move the Value part. It doesn't work for the moment because the std::pair piecewise constructor has no noexcept qualification. Is there any plan to add it ? I think adding it will force including in stl_pair.h, is it fine ? No feedback on this point ? Is using std::pair piecewise constructor ok ? If this evolution is accepted I'll adapt it for _Rb_tree that has the same problem. Working on this I also notice that content of initialization_list is not moved. Is there a plan to make initialization_list iterator type like move_iterator ? Should containers use __make_move_iterator_if_noexcept ? No. Whether to allow moving from std::initializer_list is an active topic, see http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1249r0.html Ok, nice, allowing emplace build of values would be even better, I'll have a closer look. Tested under Linux x86_64 normal mode. Ok to commit this first step ? No, this is not suitable for stage 3. It seems too risky. We can reconsider it during stage 1, but I'd like to see (at least) a new test showing a bug with the current code. For example, a type with a move constructor that is noexcept, but when used with a scoped_allocator_adaptor (which calls something other than the move constructor) we incorrectly move elements, and lose data when an exception happens. Ok, I'll try.
Re: Fix move_if_noexcept usages in _Hashtable
On 04/12/18 07:45 +0100, François Dumont wrote: Hi This patch fix a minor problem with usage of std::move_if_noexcept. We use it to move node content if move construtor is noexcept but we eventually use the allocator_type::construct method which might be slightly different. I think it is better to check for this method noexcept qualification. This is likely to pessimize some code, since most allocators do not have an exception-specification on their construct members. Moreover I have added a special overload for nodes containing a std::pair. It is supposed to allow move semantic in associative containers where Key is stored as const deleting std::pair move constructor. In this case we should still move the Value part. It doesn't work for the moment because the std::pair piecewise constructor has no noexcept qualification. Is there any plan to add it ? I think adding it will force including in stl_pair.h, is it fine ? If this evolution is accepted I'll adapt it for _Rb_tree that has the same problem. Working on this I also notice that content of initialization_list is not moved. Is there a plan to make initialization_list iterator type like move_iterator ? Should containers use __make_move_iterator_if_noexcept ? No. Whether to allow moving from std::initializer_list is an active topic, see http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1249r0.html Tested under Linux x86_64 normal mode. Ok to commit this first step ? No, this is not suitable for stage 3. It seems too risky. We can reconsider it during stage 1, but I'd like to see (at least) a new test showing a bug with the current code. For example, a type with a move constructor that is noexcept, but when used with a scoped_allocator_adaptor (which calls something other than the move constructor) we incorrectly move elements, and lose data when an exception happens.
Fix move_if_noexcept usages in _Hashtable
Hi
This patch fix a minor problem with usage of std::move_if_noexcept.
We use it to move node content if move construtor is noexcept but we
eventually use the allocator_type::construct method which might be
slightly different. I think it is better to check for this method
noexcept qualification.
Moreover I have added a special overload for nodes containing a
std::pair. It is supposed to allow move semantic in associative
containers where Key is stored as const deleting std::pair move
constructor. In this case we should still move the Value part.
It doesn't work for the moment because the std::pair piecewise
constructor has no noexcept qualification. Is there any plan to add it ?
I think adding it will force including in stl_pair.h, is it fine ?
If this evolution is accepted I'll adapt it for _Rb_tree that has the
same problem.
Working on this I also notice that content of initialization_list is
not moved. Is there a plan to make initialization_list iterator type
like move_iterator ? Should containers use
__make_move_iterator_if_noexcept ?
Tested under Linux x86_64 normal mode.
Ok to commit this first step ?
* include/bits/stl_construct.h (__construct_object_a): New.
(struct _Construct_object_a): New, use latter.
(__move_construct_object_a): New.
(struct _Move_construct_object_a): New, use latter.
(struct _Move_construct_if_noexcept_a): New.
* include/std/tuple (__move_construct_object_a): New overload for
std::pair.
* include/ext/throw_allocator.h
(throw_allocator_base<>::construct<_Up, _Args...>): Add noexcept
qualification.
* testsuite/util/testsuite_allocator.h
(tracker_allocator<>::construct<_Up, _Args...>): Likewise.
* include/bits/hashtable_policy.h: Include
(_Hashtable_alloc<>::_M_allocate_node_f): New.
(_Hashtable_alloc<>::_M_allocate_node): Adapt, use latter.
(_AllocNode<>::operator()<_Args...>): Likewise.
(_AllocNode<_NodeAlloc, _ConstructF>): Inherit from _ConstructF.
(_AllocNode<>::_M_construct_f()): New.
(_ReuseOrAllocNode<>): Inherits from _AllocNode.
* include/bits/hashtable.h:
(_Hashtable<>::__alloc_node_gen_t): New template alias to _NodeAlloc.
(_Hashtable<>::__reuse_or_alloc_node_type): Replace with...
(_Hashtable<>::__reuse_or_alloc_node_gen_t): ...that.
(_Hashtable<>::_M_assign_elements<>): Remove _NodeGenerator parameter.
(_Hashtable<>::operator=(initializer_list<>)): Adapt.
(_Hashtable<>::operator=(const _Hashtable&)): Adapt.
(_Hashtable<>::_M_move_assign(_Hashtable&&, false_type)): Adapt.
(_Hashtable<>::_Hashtable(const _Hashtable&)): Adapt.
(_Hashtable<>::_Hashtable(const _Hashtable&, const allocator_type&)):
Adapt.
(_Hashtable<>::_Hashtable(_Hashtable&&, const allocator_type&)): Adapt.
* testsuite/23_containers/unordered_map/allocator/move_assign.cc:
(test01): Add checks.
(test02): Likewise.
* testsuite/23_containers/unordered_set/allocator/move_assign.cc:
(test04): New.
François
diff --git a/libstdc++-v3/include/bits/hashtable.h b/libstdc++-v3/include/bits/hashtable.h
index 9a12ad399dc..dfb756cec07 100644
--- a/libstdc++-v3/include/bits/hashtable.h
+++ b/libstdc++-v3/include/bits/hashtable.h
@@ -266,8 +266,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_Equal, _H1, _H2, _Hash,
_RehashPolicy, _Traits>;
- using __reuse_or_alloc_node_type =
- __detail::_ReuseOrAllocNode<__node_alloc_type>;
+ template
+ using __alloc_node_gen_t =
+ __detail::_AllocNode<__node_alloc_type, _ConstructF>;
+
+ template
+ using __reuse_or_alloc_node_gen_t =
+ __detail::_ReuseOrAllocNode<__node_alloc_type, _ConstructF>;
// Metaprogramming for picking apart hash caching.
template
@@ -400,9 +405,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
// Assign *this using another _Hashtable instance. Either elements
// are copy or move depends on the _NodeGenerator.
- template
+ template
void
- _M_assign_elements(_Ht&&, const _NodeGenerator&);
+ _M_assign_elements(_Ht&&);
template
void
@@ -500,7 +505,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_Hashtable&
operator=(initializer_list __l)
{
- __reuse_or_alloc_node_type __roan(_M_begin(), *this);
+ __reuse_or_alloc_node_gen_t<_Construct_object_a>
+ __roan(*this, _M_begin());
_M_before_begin._M_nxt = nullptr;
clear();
this->_M_insert_range(__l.begin(), __l.end(), __roan, __unique_keys());
@@ -1047,9 +1053,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_M_rehash_policy = __ht._M_rehash_policy;
__try
{
- _M_assign(__ht,
- [this](const __node_type* __n)
- { return this->_M_allocate_node(__n->_M_v()); });
+ __alloc_node_gen_t<_Construct_object_a> __ang(*this);
+ _M_assign(__ht, __ang);
}
__catch(...)
{
@@ -1064,9 +1069,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
}
// Reuse allocated buckets and nodes.
- _M_assign_elements(__ht,
-
