diff mbox

Simplify non-inline function definitions for std::unordered_xxx containers

Message ID 20141203120018.GL3134@redhat.com
State New
Headers show

Commit Message

Jonathan Wakely Dec. 3, 2014, noon UTC 
While working on PR57272 for unordered containers I was getting a
headache reading all the return types with nested-name-qualifiers
split over three or four lines.

This patch replaces them with trailing return types, so the names used
in the return type are in scope and don't need to be qualified. (This
shaves over 2kB from the file as well as reducing the visual noise.)

There are also some whitespace changes not shown in this patch.

Tested x86_64-linux + powerpc64-linux, committed to trunk.

Comments

Jonathan Wakely Dec. 4, 2014, 2:11 p.m. UTC | #1 
On 04/12/14 11:39 +0000, Jonathan Wakely wrote:
>On 03/12/14 23:32 +0100, François Dumont wrote:
>>On 03/12/2014 16:59, Jonathan Wakely wrote:
>>>François (or anyone else), do you see any problem with this change?
>>>
>>>It makes the code shorter and I think is much easier to read, it also
>>>reduces the memory total shown by -ftime-report.
>>>
>>No, no problem. Interesting to see what benefit we can have when 
>>reducing number of template parameters. I will try to recall it in 
>>the future.
>
>We could remove the _Value parameter from every class template that
>also takes _Alloc, because we can get it back from _Alloc::value_type
>(assuming we consistently rebind the allocator to the value_type
>before instantiating _Hashtable, which we don't currently do).
>
>I have a patch coming soon for PR57272 that might include a change
>like that, because I already need to replace _Value with
>allocator_traits<_Alloc>::pointer everywhere.

i.e. like this. Please take a look and let me know what you think.

Although this touches almost every line of the hashtable.h and
hastable_policy.h files, it's mostly mechanical. The main purpose is
to replace every use of X* with a typedef like X_pointer, which comes
from the allocator. In the common case it's just a typedef for X*, but
the allocator can use something else. This fixes PR57272.

To make that work the _Hash_node_base and related types need to be
parameterised on the allocator's pointer, and then be fixed to not
rely on implicit conversions from pointer-to-derived to
pointer-to-base (see
http://cplusplus.github.io/LWG/lwg-active.html#2260). Once that change
is done it makes sense to replace the _Value parameter everywhere,
deriving it from the allocator instead.

(I can't help thinking the unordered container code would be much
easier to read if we just had a bundle of all the relevant template
parameters and passed that around, instead of passing the same 9
parameters to every class template!)

Something like the attached patch is necessary to fix PR57272,
although I also have an earlier version that just adds the _Ptr
parameter to the end of the template parameter list, instead of
removing _Value.  We should probably do it for GCC 5.0 if we're ever
going to do it. I have a similar patch for forward_list as well.

There is an alternative to refactoring everything to use the pointer
type, which I'm doing for std::list, std::map etc. to be
backward-compatible. That alternative is to keep the existing
non-template _Hash_node_base type but add an alternative
_Hash_node_ptr_base<_VoidPtr> which is used when the allocator has
"fancy pointers". That is extremely painful, because you need an
alternative _Hash_node<_Ptr> and _Node_iterator<_Ptr> and
_Local_iterator<_Ptr> and so on for every related type.

P.S. this patch also rewrites the __gnu_test::CustomPointerAlloc test
allocator to use a rewritten __gnu_test::CustomPointer which matches
the C++11 requirements better than __gnu_cxx::_Pointer_adapter does.
It also uses the __allocated_obj RAII type to manage
creating/destroying nodes safely without needing try/catch. Both those
changes have been very useful for all the containers when testing and
fixing PR57272.
diff mbox

Patch

commit 44a71aeae51a5b11004ff7d001ad7f3f88776777
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Dec 3 11:17:01 2014 +0000

    	* include/bits/hashtable.h: Fix whitespace and simplify function
    	definitions with trailing return types.

diff --git a/libstdc++-v3/include/bits/hashtable.h b/libstdc++-v3/include/bits/hashtable.h
index f35e7ea..369737e 100644
--- a/libstdc++-v3/include/bits/hashtable.h
+++ b/libstdc++-v3/include/bits/hashtable.h
@@ -696,8 +696,8 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       // Insert with hint, not used when keys are unique.
       template<typename _Arg, typename _NodeGenerator>
 	iterator
-	_M_insert(const_iterator, _Arg&& __arg, const _NodeGenerator& __node_gen,
-		  std::true_type __uk)
+	_M_insert(const_iterator, _Arg&& __arg,
+		  const _NodeGenerator& __node_gen, std::true_type __uk)
 	{
 	  return
 	    _M_insert(std::forward<_Arg>(__arg), __node_gen, __uk).first;
@@ -706,7 +706,8 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       // Insert with hint when keys are not unique.
       template<typename _Arg, typename _NodeGenerator>
 	iterator
-	_M_insert(const_iterator, _Arg&&, const _NodeGenerator&, std::false_type);
+	_M_insert(const_iterator, _Arg&&,
+		  const _NodeGenerator&, std::false_type);
 
       size_type
       _M_erase(std::true_type, const key_type&);
@@ -777,12 +778,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	   typename _Alloc, typename _ExtractKey, typename _Equal,
 	   typename _H1, typename _H2, typename _Hash, typename _RehashPolicy,
 	   typename _Traits>
-    typename _Hashtable<_Key, _Value, _Alloc, _ExtractKey,
-			_Equal, _H1, _H2, _Hash, _RehashPolicy,
-			_Traits>::__node_type*
+    auto
     _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
 	       _H1, _H2, _Hash, _RehashPolicy, _Traits>::
     _M_bucket_begin(size_type __bkt) const
+    -> __node_type*
     {
       __node_base* __n = _M_buckets[__bkt];
       return __n ? static_cast<__node_type*>(__n->_M_nxt) : nullptr;
@@ -851,12 +851,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	   typename _Alloc, typename _ExtractKey, typename _Equal,
 	   typename _H1, typename _H2, typename _Hash, typename _RehashPolicy,
 	   typename _Traits>
+    auto
     _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
-	       _H1, _H2, _Hash, _RehashPolicy, _Traits>&
-    _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
-	       _H1, _H2, _Hash, _RehashPolicy, _Traits>::operator=(
-		const _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
-				 _H1, _H2, _Hash, _RehashPolicy, _Traits>& __ht)
+	       _H1, _H2, _Hash, _RehashPolicy, _Traits>::
+    operator=(const _Hashtable& __ht)
+    -> _Hashtable&
     {
       if (&__ht == this)
 	return *this;
@@ -1297,12 +1296,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	   typename _Alloc, typename _ExtractKey, typename _Equal,
 	   typename _H1, typename _H2, typename _Hash, typename _RehashPolicy,
 	   typename _Traits>
-    typename _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
-			_H1, _H2, _Hash, _RehashPolicy,
-			_Traits>::iterator
+    auto
     _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
 	       _H1, _H2, _Hash, _RehashPolicy, _Traits>::
     find(const key_type& __k)
+    -> iterator
     {
       __hash_code __code = this->_M_hash_code(__k);
       std::size_t __n = _M_bucket_index(__k, __code);
@@ -1314,12 +1312,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	   typename _Alloc, typename _ExtractKey, typename _Equal,
 	   typename _H1, typename _H2, typename _Hash, typename _RehashPolicy,
 	   typename _Traits>
-    typename _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
-			_H1, _H2, _Hash, _RehashPolicy,
-			_Traits>::const_iterator
+    auto
     _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
 	       _H1, _H2, _Hash, _RehashPolicy, _Traits>::
     find(const key_type& __k) const
+    -> const_iterator
     {
       __hash_code __code = this->_M_hash_code(__k);
       std::size_t __n = _M_bucket_index(__k, __code);
@@ -1331,12 +1328,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	   typename _Alloc, typename _ExtractKey, typename _Equal,
 	   typename _H1, typename _H2, typename _Hash, typename _RehashPolicy,
 	   typename _Traits>
-    typename _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
-			_H1, _H2, _Hash, _RehashPolicy,
-			_Traits>::size_type
+    auto
     _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
 	       _H1, _H2, _Hash, _RehashPolicy, _Traits>::
     count(const key_type& __k) const
+    -> size_type
     {
       __hash_code __code = this->_M_hash_code(__k);
       std::size_t __n = _M_bucket_index(__k, __code);
@@ -1364,17 +1360,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	   typename _Alloc, typename _ExtractKey, typename _Equal,
 	   typename _H1, typename _H2, typename _Hash, typename _RehashPolicy,
 	   typename _Traits>
-    std::pair<typename _Hashtable<_Key, _Value, _Alloc,
-				  _ExtractKey, _Equal, _H1,
-				  _H2, _Hash, _RehashPolicy,
-				  _Traits>::iterator,
-	      typename _Hashtable<_Key, _Value, _Alloc,
-				  _ExtractKey, _Equal, _H1,
-				  _H2, _Hash, _RehashPolicy,
-				  _Traits>::iterator>
+    auto
     _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
 	       _H1, _H2, _Hash, _RehashPolicy, _Traits>::
     equal_range(const key_type& __k)
+    -> pair<iterator, iterator>
     {
       __hash_code __code = this->_M_hash_code(__k);
       std::size_t __n = _M_bucket_index(__k, __code);
@@ -1397,17 +1387,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	   typename _Alloc, typename _ExtractKey, typename _Equal,
 	   typename _H1, typename _H2, typename _Hash, typename _RehashPolicy,
 	   typename _Traits>
-    std::pair<typename _Hashtable<_Key, _Value, _Alloc,
-				  _ExtractKey, _Equal, _H1,
-				  _H2, _Hash, _RehashPolicy,
-				  _Traits>::const_iterator,
-	      typename _Hashtable<_Key, _Value, _Alloc,
-				  _ExtractKey, _Equal, _H1,
-				  _H2, _Hash, _RehashPolicy,
-				  _Traits>::const_iterator>
+    auto
     _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
 	       _H1, _H2, _Hash, _RehashPolicy, _Traits>::
     equal_range(const key_type& __k) const
+    -> pair<const_iterator, const_iterator>
     {
       __hash_code __code = this->_M_hash_code(__k);
       std::size_t __n = _M_bucket_index(__k, __code);
@@ -1432,13 +1416,12 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	   typename _Alloc, typename _ExtractKey, typename _Equal,
 	   typename _H1, typename _H2, typename _Hash, typename _RehashPolicy,
 	   typename _Traits>
-    typename _Hashtable<_Key, _Value, _Alloc, _ExtractKey,
-			_Equal, _H1, _H2, _Hash, _RehashPolicy,
-			_Traits>::__node_base*
+    auto
     _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
 	       _H1, _H2, _Hash, _RehashPolicy, _Traits>::
     _M_find_before_node(size_type __n, const key_type& __k,
 			__hash_code __code) const
+    -> __node_base*
     {
       __node_base* __prev_p = _M_buckets[__n];
       if (!__prev_p)
@@ -1516,12 +1499,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	   typename _Alloc, typename _ExtractKey, typename _Equal,
 	   typename _H1, typename _H2, typename _Hash, typename _RehashPolicy,
 	   typename _Traits>
-    typename _Hashtable<_Key, _Value, _Alloc, _ExtractKey,
-			_Equal, _H1, _H2, _Hash, _RehashPolicy,
-			_Traits>::__node_base*
+    auto
     _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
 	       _H1, _H2, _Hash, _RehashPolicy, _Traits>::
     _M_get_previous_node(size_type __bkt, __node_base* __n)
+    -> __node_base*
     {
       __node_base* __prev_n = _M_buckets[__bkt];
       while (__prev_n->_M_nxt != __n)
@@ -1534,13 +1516,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	   typename _H1, typename _H2, typename _Hash, typename _RehashPolicy,
 	   typename _Traits>
     template<typename... _Args>
-      std::pair<typename _Hashtable<_Key, _Value, _Alloc,
-				    _ExtractKey, _Equal, _H1,
-				    _H2, _Hash, _RehashPolicy,
-				    _Traits>::iterator, bool>
+      auto
       _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
 		 _H1, _H2, _Hash, _RehashPolicy, _Traits>::
       _M_emplace(std::true_type, _Args&&... __args)
+      -> pair<iterator, bool>
       {
 	// First build the node to get access to the hash code
 	__node_type* __node = this->_M_allocate_node(std::forward<_Args>(__args)...);
@@ -1574,12 +1554,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	   typename _H1, typename _H2, typename _Hash, typename _RehashPolicy,
 	   typename _Traits>
     template<typename... _Args>
-      typename _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
-			  _H1, _H2, _Hash, _RehashPolicy,
-			  _Traits>::iterator
+      auto
       _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
 		 _H1, _H2, _Hash, _RehashPolicy, _Traits>::
       _M_emplace(const_iterator __hint, std::false_type, _Args&&... __args)
+      -> iterator
       {
 	// First build the node to get its hash code.
 	__node_type* __node =
@@ -1603,13 +1582,12 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	   typename _Alloc, typename _ExtractKey, typename _Equal,
 	   typename _H1, typename _H2, typename _Hash, typename _RehashPolicy,
 	   typename _Traits>
-    typename _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
-			_H1, _H2, _Hash, _RehashPolicy,
-			_Traits>::iterator
+    auto
     _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
 	       _H1, _H2, _Hash, _RehashPolicy, _Traits>::
     _M_insert_unique_node(size_type __bkt, __hash_code __code,
 			  __node_type* __node)
+    -> iterator
     {
       const __rehash_state& __saved_state = _M_rehash_policy._M_state();
       std::pair<bool, std::size_t> __do_rehash
@@ -1643,13 +1621,12 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	   typename _Alloc, typename _ExtractKey, typename _Equal,
 	   typename _H1, typename _H2, typename _Hash, typename _RehashPolicy,
 	   typename _Traits>
-    typename _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
-			_H1, _H2, _Hash, _RehashPolicy,
-			_Traits>::iterator
+    auto
     _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
 	       _H1, _H2, _Hash, _RehashPolicy, _Traits>::
     _M_insert_multi_node(__node_type* __hint, __hash_code __code,
 			 __node_type* __node)
+    -> iterator
     {
       const __rehash_state& __saved_state = _M_rehash_policy._M_state();
       std::pair<bool, std::size_t> __do_rehash
@@ -1709,13 +1686,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	   typename _H1, typename _H2, typename _Hash, typename _RehashPolicy,
 	   typename _Traits>
     template<typename _Arg, typename _NodeGenerator>
-      std::pair<typename _Hashtable<_Key, _Value, _Alloc,
-				    _ExtractKey, _Equal, _H1,
-				    _H2, _Hash, _RehashPolicy,
-				    _Traits>::iterator, bool>
+      auto
       _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
 		 _H1, _H2, _Hash, _RehashPolicy, _Traits>::
       _M_insert(_Arg&& __v, const _NodeGenerator& __node_gen, std::true_type)
+      -> pair<iterator, bool>
       {
 	const key_type& __k = this->_M_extract()(__v);
 	__hash_code __code = this->_M_hash_code(__k);
@@ -1735,14 +1710,12 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	   typename _H1, typename _H2, typename _Hash, typename _RehashPolicy,
 	   typename _Traits>
     template<typename _Arg, typename _NodeGenerator>
-      typename _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
-			  _H1, _H2, _Hash, _RehashPolicy,
-			  _Traits>::iterator
+      auto
       _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
 		 _H1, _H2, _Hash, _RehashPolicy, _Traits>::
       _M_insert(const_iterator __hint, _Arg&& __v,
-		const _NodeGenerator& __node_gen,
-		std::false_type)
+		const _NodeGenerator& __node_gen, std::false_type)
+      -> iterator
       {
 	// First compute the hash code so that we don't do anything if it
 	// throws.
@@ -1758,12 +1731,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	   typename _Alloc, typename _ExtractKey, typename _Equal,
 	   typename _H1, typename _H2, typename _Hash, typename _RehashPolicy,
 	   typename _Traits>
-    typename _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
-			_H1, _H2, _Hash, _RehashPolicy,
-			_Traits>::iterator
+    auto
     _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
 	       _H1, _H2, _Hash, _RehashPolicy, _Traits>::
     erase(const_iterator __it)
+    -> iterator
     {
       __node_type* __n = __it._M_cur;
       std::size_t __bkt = _M_bucket_index(__n);
@@ -1779,12 +1751,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	   typename _Alloc, typename _ExtractKey, typename _Equal,
 	   typename _H1, typename _H2, typename _Hash, typename _RehashPolicy,
 	   typename _Traits>
-    typename _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
-			_H1, _H2, _Hash, _RehashPolicy,
-			_Traits>::iterator
+    auto
     _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
 	       _H1, _H2, _Hash, _RehashPolicy, _Traits>::
     _M_erase(size_type __bkt, __node_base* __prev_n, __node_type* __n)
+    -> iterator
     {
       if (__prev_n == _M_buckets[__bkt])
 	_M_remove_bucket_begin(__bkt, __n->_M_next(),
@@ -1808,12 +1779,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	   typename _Alloc, typename _ExtractKey, typename _Equal,
 	   typename _H1, typename _H2, typename _Hash, typename _RehashPolicy,
 	   typename _Traits>
-    typename _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
-			_H1, _H2, _Hash, _RehashPolicy,
-			_Traits>::size_type
+    auto
     _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
 	       _H1, _H2, _Hash, _RehashPolicy, _Traits>::
     _M_erase(std::true_type, const key_type& __k)
+    -> size_type
     {
       __hash_code __code = this->_M_hash_code(__k);
       std::size_t __bkt = _M_bucket_index(__k, __code);
@@ -1833,12 +1803,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	   typename _Alloc, typename _ExtractKey, typename _Equal,
 	   typename _H1, typename _H2, typename _Hash, typename _RehashPolicy,
 	   typename _Traits>
-    typename _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
-			_H1, _H2, _Hash, _RehashPolicy,
-			_Traits>::size_type
+    auto
     _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
 	       _H1, _H2, _Hash, _RehashPolicy, _Traits>::
     _M_erase(std::false_type, const key_type& __k)
+    -> size_type
     {
       __hash_code __code = this->_M_hash_code(__k);
       std::size_t __bkt = _M_bucket_index(__k, __code);
@@ -1890,12 +1859,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	   typename _Alloc, typename _ExtractKey, typename _Equal,
 	   typename _H1, typename _H2, typename _Hash, typename _RehashPolicy,
 	   typename _Traits>
-    typename _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
-			_H1, _H2, _Hash, _RehashPolicy,
-			_Traits>::iterator
+    auto
     _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
 	       _H1, _H2, _Hash, _RehashPolicy, _Traits>::
     erase(const_iterator __first, const_iterator __last)
+    -> iterator
     {
       __node_type* __n = __first._M_cur;
       __node_type* __last_n = __last._M_cur;