Fix move_if_noexcept usages in _Hashtable

[François Dumont]

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 <tuple> 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 <bits/stl_construct.h>
    (_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

[Jonathan Wakely]

Post by François Dumont
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.

Post by François Dumont
  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 <tuple> 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

Post by François Dumont
  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.

[François Dumont]

Post by Jonathan Wakely

Post by François Dumont
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.

Post by Jonathan Wakely

Post by François Dumont
  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 <tuple> in stl_pair.h, is
it fine ?

No feedback on this point ? Is using std::pair piecewise constructor ok ?

Post by Jonathan Wakely

Post by François Dumont
  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.

Post by Jonathan Wakely

Post by François Dumont
  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.