Hashtable comment cleanups & renamings

[François Dumont]

Here is a simplified form.

 Â Â Â  Rename variables and cleanup comments.
 Â Â Â  * include/bits/hashtable_policy.h
 Â Â Â  * include/bits/hashtable.h

Ok to commit ?

François

On 5/17/19 10:24 PM, Jonathan Wakely wrote:
> On 17/05/19 18:19 +0200, François Dumont wrote:
>> Hi
>>
>> Â Â Â  I got tired of '__n' being used in _Hashtable for many different 
>> purposes: node, bucket, bucket count, bucket hint. It makes the code 
>> difficult to read. This code makes sure that __n is a node except is 
>> some very limited use cases where the method name is clear enough to 
>> tell what __n means.
>>
>> Â Â Â  So I'd like to commit this patch which only change that and some 
>> comments before moving forward to more serious stuff. The only code 
>> change is a use of auto return type on _M_allocate_node.
>>
>> Â Â Â  My main concern is the ChangeLog entry. Is the following entry ok ?
>>
>> Â Â Â  Rename variables and cleanup comments.
>> Â Â Â  * include/bits/hashtable_policy.h
>> Â Â Â  * include/bits/hashtable.h
>>
>> Â Â Â  Tested under Linux x86_64 (even if it can't be otherwise)
>>
>> François
>>
>
>> @@ -350,24 +347,24 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> Â Â Â Â Â  _M_base_alloc() { return *this; }
>>
>> Â Â Â Â Â  __bucket_type*
>> -Â Â Â Â Â  _M_allocate_buckets(size_type __n)
>> +Â Â Â Â Â  _M_allocate_buckets(size_type __bkt_count)
>
> This is a much more helpful name, thanks.
>
>> @@ -439,30 +436,31 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> Â Â Â Â Â  { }
>>
>> Â Â Â Â Â  explicit
>> -Â Â Â Â Â  _Hashtable(size_type __n,
>> +Â Â Â Â Â  _Hashtable(size_type __bkt_hint,
>
> This seems less helpful. Would __num_bkts_hint be clearer?
> Or for consistency, __bkt_count_hint?
>
>> @@ -1415,9 +1414,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> Â Â Â  -> iterator
>> Â Â Â  {
>> Â Â Â Â Â  __hash_code __code = this->_M_hash_code(__k);
>> -Â Â Â Â Â  std::size_t __n = _M_bucket_index(__k, __code);
>> -Â Â Â Â Â  __node_type* __p = _M_find_node(__n, __k, __code);
>> -Â Â Â Â Â  return __p ? iterator(__p) : end();
>> +Â Â Â Â Â  std::size_t __bkt = _M_bucket_index(__k, __code);
>> +Â Â Â Â Â  __node_type* __n = _M_find_node(__bkt, __k, __code);
>> +Â Â Â Â Â  return __n ? iterator(__n) : end();
>
> Is __n really an improvement over __p here?
>
> If you're changing it, __node or __ptr might be an improvement, but
> changing __p to __n seems like unnecessary churn.
>
> I'm not convinced that __n is a big enough improvement over __p to
> bother changing dozens of lines, for not much benefit. All those
> changes will make it slower to use git blame to track down when thigns
> changed, and will make it harder to review diffs between trunk and
> older branches.
>
>
>> @@ -1479,17 +1478,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> Â Â Â  -> pair<iterator, iterator>
>> Â Â Â  {
>> Â Â Â Â Â  __hash_code __code = this->_M_hash_code(__k);
>> -Â Â Â Â Â  std::size_t __n = _M_bucket_index(__k, __code);
>> -Â Â Â Â Â  __node_type* __p = _M_find_node(__n, __k, __code);
>> +Â Â Â Â Â  std::size_t __bkt = _M_bucket_index(__k, __code);
>> +Â Â Â Â Â  __node_type* __n = _M_find_node(__bkt, __k, __code);
>>
>> -Â Â Â Â Â  if (__p)
>> +Â Â Â Â Â  if (__n)
>> Â Â Â Â {
>> -Â Â Â Â Â  __node_type* __p1 = __p->_M_next();
>> -Â Â Â Â Â  while (__p1 && _M_bucket_index(__p1) == __n
>> -Â Â Â Â Â Â Â Â  && this->_M_equals(__k, __code, __p1))
>> -Â Â Â Â Â Â Â  __p1 = __p1->_M_next();
>> +Â Â Â Â Â  __node_type* __n1 = __n->_M_next();
>
> __p1 is not a good name, but __n1 is no better.
>
> At least with __p the second pointer could be __q, which is a fairly
> idiomatic pairing of letters :-)
>
> How about __first and __last? Or __n and __next?  Even __n1 and __n2
> seems better than __n and __n1. Those pointers end up being used for
> the 'first' and 'second' members of a pair, so __n1 and __n2 makes
> more sense than setting 'first' from __n and 'second' from __n1.
>
> But I don't feel strongly about it, so if it's just me who dislikes
> __n and __n1 then it doesn't matter.
>
>> diff --git a/libstdc++-v3/include/bits/hashtable_policy.h 
>> b/libstdc++-v3/include/bits/hashtable_policy.h
>> index a4d2a97f4f3..bb2e7b762ff 100644
>> --- a/libstdc++-v3/include/bits/hashtable_policy.h
>> +++ b/libstdc++-v3/include/bits/hashtable_policy.h
>> @@ -181,7 +181,7 @@ namespace __detail
>>    *  @tparam _Cache_hash_code  Boolean value. True if the value of
>> Â Â  *Â  the hash function is stored along with the value. This is a
>>    *  time-space tradeoff.  Storing it may improve lookup speed by
>> -Â Â  *Â  reducing the number of times we need to call the _Equal
>> +Â Â  *Â  reducing the number of times we need to call the _Hash
>
> Doesn't it reduce both?
>
> In _M_equals we don't bother calling the _Equal predicate if the
> cached hash code doesn't match the one for the key we're comparing.
>
>
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: hashtable_renames.patch
Type: text/x-patch
Size: 34315 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/libstdc++/attachments/20190521/f416c225/attachment.bin>