[PATCH][Hashtable 5/6] Remove H1/H2 template parameters

[Jonathan Wakely]

On 26/08/20 16:58 +0100, Jonathan Wakely wrote:
>On 26/08/20 16:55 +0100, Jonathan Wakely wrote:
>>On 26/08/20 16:30 +0100, Jonathan Wakely wrote:
>>>I'm seeing new FAILures with this:
>>>
>>>FAIL: 20_util/function_objects/searchers.cc (test for excess errors)
>>>UNRESOLVED: 20_util/function_objects/searchers.cc compilation failed to produce executable
>>>FAIL: experimental/functional/searchers.cc (test for excess errors)
>>>UNRESOLVED: experimental/functional/searchers.cc compilation failed to produce executable
>>>
>>>It looks like what you committed is not what you sent for review. The
>>>patch sent for review has:
>>>
>>> /// Specialization: hash function and range-hashing function, no
>>> /// caching of hash codes.
>>> /// Provides typedef and accessor required by C++ 11.
>>> template<typename _Key, typename _Value, typename _ExtractKey,
>>>-          typename _H1, typename _H2>
>>>-    struct _Hash_code_base<_Key, _Value, _ExtractKey, _H1, _H2,
>>>-                          _Default_ranged_hash, false>
>>>+          typename _Hash, typename _RangeHash, typename _Unused>
>>>+    struct _Hash_code_base<_Key, _Value, _ExtractKey, _Hash, _RangeHash,
>>>+                          _Unused, false>
>>>   : private _Hashtable_ebo_helper<0, _ExtractKey>,
>>>-      private _Hashtable_ebo_helper<1, _H1>,
>>>-      private _Hashtable_ebo_helper<2, _H2>
>>>+      private _Hashtable_ebo_helper<1, _Hash>
>>>   {
>>>
>>>
>>>But what you committed has:
>>>
>>> /// Specialization: hash function and range-hashing function, no
>>> /// caching of hash codes.
>>> /// Provides typedef and accessor required by C++ 11.
>>> template<typename _Key, typename _Value, typename _ExtractKey,
>>>-          typename _H1, typename _H2>
>>>-    struct _Hash_code_base<_Key, _Value, _ExtractKey, _H1, _H2,
>>>-                          _Default_ranged_hash, false>
>>>-    : private _Hashtable_ebo_helper<0, _ExtractKey>,
>>>-      private _Hashtable_ebo_helper<1, _H1>,
>>>-      private _Hashtable_ebo_helper<2, _H2>
>>>+          typename _Hash, typename _RangeHash, typename _Unused>
>>>+    struct _Hash_code_base<_Key, _Value, _ExtractKey, _Hash, _RangeHash,
>>>+                          _Unused, false>
>>>+    : private _Hashtable_ebo_helper<0, _Hash>
>>>   {
>>>
>>>
>>>Note that you've changed the type of the base class from:
>>>
>>>+      private _Hashtable_ebo_helper<1, _Hash>
>>>
>>>to
>>>
>>>+      private _Hashtable_ebo_helper<0, _Hash>
>>>
>>>This causes an ambiguity:
>>>
>>>/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/hashtable_policy.h:1706: error: 'std::__detail::_Hashtable_ebo_helper<0, test03()::<unnamed struct>, true>' is an ambiguous base of 'std::__detail::_Hashtable_base<char, std::pair<const char, long int>, std::__detail::_Select1st, test03()::<unnamed struct>, test03()::<unnamed struct>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Hashtable_traits<true, false, true> >'
>>>
>>>However, what I don't understand is why we are storing that _Hash type
>>>more than once as a base class. That seems wrong (but not something we
>>>can change without ABI impact).
>>
>>Ah, we're not storing it more than once.
>>
>>The problem is:
>>
>> template<typename _Key, typename _Value,
>>	   typename _ExtractKey, typename _Equal,
>>	   typename _H1, typename _H2, typename _Hash, typename _Traits>
>> struct _Hashtable_base
>> : public _Hash_code_base<_Key, _Value, _ExtractKey, _H1, _H2, _Hash,
>>			   _Traits::__hash_cached::value>,
>>   private _Hashtable_ebo_helper<0, _Equal>
>>
>>This has a base of _Hashtable_ebo_helper<0, _Equal> so it used to
>>have these bases:
>>
>>_Hashtable_ebo_helper<0, _ExtractKey>
>>_Hashtable_ebo_helper<1, _Hash>
>>_Hashtable_ebo_helper<2, _RangeHash>
>>_Hashtable_ebo_helper<0, _Equal>
>>
>>but after your change it has these bases:
>>
>>_Hashtable_ebo_helper<0, _Hash>
>>_Hashtable_ebo_helper<0, _Equal>
>>
>>In the case
>>where _Equal and _Hash are the same type (which is what I was testing
>>in the test that fail, because I'm sneaky) that means:
>>
>>_Hashtable_ebo_helper<0, T>
>>_Hashtable_ebo_helper<0, T>
>>
>>which is obviously ambiguous.
>>
>>I think the _hash_code_base should still use the index 1 for its base
>>class, i.e. _Hashtable_ebo_helper<1, _Hash>. That way we have these:
>>
>>_Hashtable_ebo_helper<1, _Hash>
>>_Hashtable_ebo_helper<0, _Equal>
>>
>>which works even if they're the same types.
>
>Here's a minimal test:
>
>#include <unordered_map>
>
>struct Evil : std::hash<int>, std::equal_to<int>
>{
>};
>
>int main()
>{
>  std::unordered_map<int, int, Evil, Evil> h;
>  h.key_eq();
>}
>
>This fails on current trunk.

Fixed by the attached patch.

Tested powerpc64le-linux, committed to trunk.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.txt
Type: text/x-patch
Size: 3306 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20200826/223c64f2/attachment-0001.bin>