diff mbox series

[02/12] libstdc++: Allow unordered_set assignment to assign to existing nodes

Message ID 20241108154548.813315-3-jwakely@redhat.com
State New
Headers show
Series libstdc++: Refactor _Hashtable class | expand

Commit Message

Jonathan Wakely Nov. 8, 2024, 3:30 p.m. UTC
Currently the _ReuseOrAllocNode::operator(Args&&...) function always
destroys the value stored in recycled nodes and constructs a new value.

The _ReuseOrAllocNode type is only ever used for implementing
assignment, either from another unordered container of the same type, or
from std::initializer_list<value_type>. Consequently, the parameter pack
Args only ever consists of a single parameter or type const value_type&
or value_type.  We can replace the variadic parameter pack with a single
forwarding reference parameter, and when the value_type is assignable
from that type we can use assignment instead of destroying the existing
value and then constructing a new one.

Using assignment is typically only possible for sets, because for maps
the value_type is std::pair<const key_type, mapped_type> and in most
cases std::is_assignable_v<const key_type&, const key_type&> is false.

libstdc++-v3/ChangeLog:

	* include/bits/hashtable_policy.h (_ReuseOrAllocNode::operator()):
	Replace parameter pack with a single parameter. Assign to
	existing value when possible.
	* testsuite/23_containers/unordered_multiset/allocator/move_assign.cc:
	Adjust expected count of operations.
	* testsuite/23_containers/unordered_set/allocator/move_assign.cc:
	Likewise.

Reviewed-by: François Dumont <fdumont@gcc.gnu.org>
---
 libstdc++-v3/include/bits/hashtable_policy.h  | 37 +++++++++++++------
 .../allocator/move_assign.cc                  |  5 ++-
 .../unordered_set/allocator/move_assign.cc    | 10 +++--
 3 files changed, 35 insertions(+), 17 deletions(-)
diff mbox series

Patch

diff --git a/libstdc++-v3/include/bits/hashtable_policy.h b/libstdc++-v3/include/bits/hashtable_policy.h
index b5f837e6061..7a3c66c37fd 100644
--- a/libstdc++-v3/include/bits/hashtable_policy.h
+++ b/libstdc++-v3/include/bits/hashtable_policy.h
@@ -172,24 +172,39 @@  namespace __detail
       ~_ReuseOrAllocNode()
       { _M_h._M_deallocate_nodes(_M_nodes); }
 
-      template<typename... _Args>
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wc++17-extensions" // if constexpr
+      template<typename _Arg>
 	__node_ptr
-	operator()(_Args&&... __args)
+	operator()(_Arg&& __arg)
 	{
 	  if (!_M_nodes)
-	    return _M_h._M_allocate_node(std::forward<_Args>(__args)...);
+	    return _M_h._M_allocate_node(std::forward<_Arg>(__arg));
+
+	  using value_type = typename _NodeAlloc::value_type::value_type;
 
 	  __node_ptr __node = _M_nodes;
-	  _M_nodes = _M_nodes->_M_next();
-	  __node->_M_nxt = nullptr;
-	  auto& __a = _M_h._M_node_allocator();
-	  __node_alloc_traits::destroy(__a, __node->_M_valptr());
-	  _NodePtrGuard<__hashtable_alloc, __node_ptr> __guard { _M_h, __node };
-	  __node_alloc_traits::construct(__a, __node->_M_valptr(),
-					 std::forward<_Args>(__args)...);
-	  __guard._M_ptr = nullptr;
+	  if constexpr (is_assignable<value_type&, _Arg>::value)
+	    {
+	      __node->_M_v() = std::forward<_Arg>(__arg);
+	      _M_nodes = _M_nodes->_M_next();
+	      __node->_M_nxt = nullptr;
+	    }
+	  else
+	    {
+	      _M_nodes = _M_nodes->_M_next();
+	      __node->_M_nxt = nullptr;
+	      auto& __a = _M_h._M_node_allocator();
+	      __node_alloc_traits::destroy(__a, __node->_M_valptr());
+	      _NodePtrGuard<__hashtable_alloc, __node_ptr>
+		__guard{ _M_h, __node };
+	      __node_alloc_traits::construct(__a, __node->_M_valptr(),
+					     std::forward<_Arg>(__arg));
+	      __guard._M_ptr = nullptr;
+	    }
 	  return __node;
 	}
+#pragma GCC diagnostic pop
 
     private:
       __node_ptr _M_nodes;
diff --git a/libstdc++-v3/testsuite/23_containers/unordered_multiset/allocator/move_assign.cc b/libstdc++-v3/testsuite/23_containers/unordered_multiset/allocator/move_assign.cc
index 50608ec443f..6d00354902e 100644
--- a/libstdc++-v3/testsuite/23_containers/unordered_multiset/allocator/move_assign.cc
+++ b/libstdc++-v3/testsuite/23_containers/unordered_multiset/allocator/move_assign.cc
@@ -46,8 +46,9 @@  void test01()
   VERIFY( 1 == v1.get_allocator().get_personality() );
   VERIFY( 2 == v2.get_allocator().get_personality() );
 
-  VERIFY( counter_type::move_count == 1  );
-  VERIFY( counter_type::destructor_count == 2 );
+  VERIFY( counter_type::move_count == 0  );
+  // 1 element in v1 destroyed.
+  VERIFY( counter_type::destructor_count == 1 );
 }
 
 void test02()
diff --git a/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc b/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc
index 677ea67d0ea..6be70022705 100644
--- a/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc
+++ b/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc
@@ -52,8 +52,9 @@  void test01()
     VERIFY( 1 == v1.get_allocator().get_personality() );
     VERIFY( 2 == v2.get_allocator().get_personality() );
 
-    VERIFY( counter_type::move_count == 1  );
-    VERIFY( counter_type::destructor_count == 2 );
+    VERIFY( counter_type::move_count == 0  );
+  // 1 element in v1 destroyed.
+    VERIFY( counter_type::destructor_count == 1 );
   }
 
   // Check there's nothing left allocated or constructed.
@@ -130,8 +131,9 @@  void test03()
     VERIFY( 1 == v1.get_allocator().get_personality() );
     VERIFY( 2 == v2.get_allocator().get_personality() );
 
-    VERIFY( counter_type::move_count == 1  );
-    VERIFY( counter_type::destructor_count == i + 1 );
+    VERIFY( counter_type::move_count == 0  );
+    // (i - 1) elements in v2 destroyed, and 1 element in v1 destroyed.
+    VERIFY( counter_type::destructor_count == i );
   }
 
   // Check there's nothing left allocated or constructed.