Re: [PATCH] PR libstdc++/92124 on hashtable

[Jonathan Wakely]

On 07/11/19 20:28 +0100, François Dumont wrote:

From what I understood from recent fix the unordered containers need to be updated the same way.

I hope you'll appreciate the usage of rvalue forwarding. Containers node values are moved as soon as _M_assign is called with a rvalue reference to the source container.

Additionnaly this patch removes usages of lambdas in _Hashtable.

If you confirm it I'll check for the same on _Rb_tree.

    * include/bits/hashtable.h (_Hashtable<>::__alloc_node_gen_t): New
    template alias.
    (_Hashtable<>::__mv_if_value_type_mv_noexcept): New.
    (_Hashtable<>::__fwd_value): New.
    (_Hashtable<>::_M_assign_elements<>): Remove _NodeGenerator template
    parameter.
    (_Hashtable<>::_M_assign<>): Add _Ht template parameter.
    (_Hashtable<>::operator=(const _Hashtable<>&)): Adapt.
    (_Hashtable<>::_M_move_assign): 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_set/92124.cc: New.

Tested under Linux x86_64.

Ok to commit ?

François

diff --git a/libstdc++-v3/include/bits/hashtable.h 
b/libstdc++-v3/include/bits/hashtable.h
index ab579a7059e..c2b2219d471 100644
--- a/libstdc++-v3/include/bits/hashtable.h
+++ b/libstdc++-v3/include/bits/hashtable.h
@@ -255,6 +255,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION

      using __reuse_or_alloc_node_gen_t =
        __detail::_ReuseOrAllocNode<__node_alloc_type>;
+      using __alloc_node_gen_t =
+       __detail::_AllocNode<__node_alloc_type>;

      // Simple RAII type for managing a node containing an element
      struct _Scoped_node
@@ -280,6 +282,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
        __node_type* _M_node;
      };

+      template<typename _Tp>
+       static constexpr
+       typename conditional<__move_if_noexcept_cond<value_type>::value,
+                            const _Tp&, _Tp&&>::type
+       __mv_if_value_type_mv_noexcept(_Tp& __x) noexcept
+       { return std::move(__x); }
+
+      template<typename _Ht>
+       static constexpr
+       typename conditional<!std::is_lvalue_reference<_Ht>::value,
+                            value_type&&, const value_type&>::type
+       __fwd_value(_Ht&&, value_type& __val) noexcept
+       { return std::move(__val); }
+
      // Metaprogramming for picking apart hash caching.
      template<typename _Cond>
        using __if_hash_cached = __or_<__not_<__hash_cached>, _Cond>;
@@ -406,13 +422,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION

      // Assign *this using another _Hashtable instance. Either elements
      // are copy or move depends on the _NodeGenerator.

N.B. I know this comment was already there, but could you please
change the second sentence to say:

"Whether elements are copied or moved ..."

-      template<typename _Ht, typename _NodeGenerator>
+      template<typename _Ht>
        void
-       _M_assign_elements(_Ht&&, const _NodeGenerator&);
+       _M_assign_elements(_Ht&&);

-      template<typename _NodeGenerator>
+      template<typename _Ht, typename _NodeGenerator>
        void
-       _M_assign(const _Hashtable&, const _NodeGenerator&);
+       _M_assign(_Ht&&, const _NodeGenerator&);

      void
      _M_move_assign(_Hashtable&&, true_type);