Re: hashtable local iterator

[Jonathan Wakely <jwakely dot gcc at gmail dot com>]

On 21 December 2011 01:27, Paolo Carlini wrote:
> .. by the way, when working on this issue, make sure to have 'PR
> libstdc++/51608' on the ChangeLog, and please also double check the latter
> for typos (I see an 'unordere_map.H')

And "whether hash code is cache or not" should be "cached" not "cache"

There should be a new line after that, before "(_Hash_code_base):"


Thanks for commenting the code thoroughly, the code is quite
complicated, so good comments are very important.  I haven't studied
the code closely enough to understand it all but there are several
grammatical issues in the comments that made it harder for me.


       static _Base_local_iterator
       _S_to_local(_Base_iterator __it)
-      { return _Base_local_iterator(__it._M_cur); }
+      {
+	// The return local iterator is not going to be incremented so we don't
+	// need to compute __it node bucket
+	return _Base_local_iterator(__it._M_cur, 0, 0);
+      }

I didn't find it easy to understand this comment, maybe it would be clearer as:

"The returned local iterator will not be incremented so we don't need
to compute __it's node bucket"

or maybe

"The returned local iterator will not be incremented so doesn't need
to know the correct node bucket"


+      // Following two static assertions are necessary to guaranty
that swapping

"guarantee"

+      // When hash codes are cached local iterator only use H2 that
then have to
+      // be empty.

Could that comment be made clearer too? maybe "only uses H2, which
must then be empty"


+      static_assert(__or_<__not_<integral_constant<bool, __cache_hash_code>>,
+			  is_empty<_H2>>::value,
+	    "Functor used to map hash code on bucket index must be empty");
+

Should that be "map to" not "map on"?

I think all those static assertions would be much easier to read using
something like:

    template<typename _Tp>
      using __if_cached = __or_<__not_<integral_constant<bool,
__cache_hash_code>>, _Tp>;

    template<typename _Tp>
      using __if_not_cached = __or_<integral_constant<bool,
__cach_hash_code>, _Tp>;

static_assert( __if_cached<is_empty<H2>>::value, "..." );


+  // Node iterators, used to iterate through all the hashtable.

Maybe "through the entire hashtable" ?


+  // Each specialzation use inheritance with one or more of the template

specialization, not specialzation
uses, not use

+  // parameters depending on its needs to benefit from EBO. This is

I would rewrite that first sentence as "Each specialization derives
from one or more of the template parameters to benefit from EBO." I
don't think "depending on its needs" really adds value.

I'm still not clear why tuple can't be used to exploit the EBO.

+  // important as this type is inherited in some cases by the
+  // _Local_iterator_base type used to implement local_iterator and
+  // const_local_iterator. As any iterator type we prefer to make it as
+  // small as possible.

"As with any" not "As any"


   // Specialization: hash function and range-hashing function, no
   // caching of hash codes.  H is provided but ignored.  Provides
   // typedef and accessor required by TR1.

What is H here?