Hashtable comment cleanups & renamings

[François Dumont]

I had miss some occurences of __bucket_hint to replace with 
__bucket_count_hint so here is a new version.

Ok to commit with the simple ChangeLog entry below ?


On 5/21/19 7:42 AM, François Dumont wrote:
> 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: 33664 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/libstdc++/attachments/20190527/ecf1b2fb/attachment.bin>