[PATCH][Hashtable 0/6] Code review

[François Dumont]

On 17/07/20 12:11 pm, Jonathan Wakely wrote:
> N.B. the 0/6 entry of a patch series is supposed to be a cover letter
> describing the changes in the series, not one of the patches.
>
> If you have patches 0/6, 1/6, 2/6 ... 6/6 then you actually have seven
> patches, not six!
>
> Anyway ...
>
> On 19/12/19 20:17 +0100, François Dumont wrote:
>> diff --git a/libstdc++-v3/include/bits/hashtable_policy.h 
>> b/libstdc++-v3/include/bits/hashtable_policy.h
>> index f5809c7443a..b9320506bb2 100644
>> --- a/libstdc++-v3/include/bits/hashtable_policy.h
>> +++ b/libstdc++-v3/include/bits/hashtable_policy.h
>> @@ -172,6 +172,14 @@ namespace __detail
>>
>>   // Auxiliary types used for all instantiations of _Hashtable nodes
>>   // and iterators.
>> +  using __unique_keys_t = true_type;
>> +  using __multi_keys_t = false_type;
>> +
>> +  using __constant_iterators_t = true_type;
>> +  using __mutable_iterators_t = false_type;
>> +
>> +  using __hash_cached_t = true_type;
>> +  using __hash_not_cached_t = false_type;
>
> This is an ABI change, and the benefit doesn't seem large enough to
> justify it. It will result in code size increases for anything that
> links objects compiled before and after the change.
>
> If we wanted to do this, I think it would be better to use enums, so:
>
> enum _Unique_keys { __unique_keys, __multi_keys };
>
> Otherwise you can use __hash_cached_t where __unique_keys_t is meant
> to be used, so all you've done is introduce more names for the same
> things.
>
> If you want to replace the 'true' and 'false' literals with something
> more descriptive, can't you just use constants?
>
> constexpr bool __hash_cached = true;
> constexpr bool __hash_not_cached = false;
>
> Or just use comments:
>
>     struct _Local_iterator_base<_Key, _Value, _ExtractKey,
>                 _H1, _H2, _Hash, /*cached=*/ false>
>
>
Ok, I'll review this patch with this simpler approach.

I even started to add comments in the patch you already approved.