Deeply sorry, I indeed didn't sent the patch I wanted to commit. It was in 3 commits on my side and it looks like I had miss the last one regarding the changes for _ExtractKey.

But moreover I had change the ebo helper index wrongly, I missed the abi change here, thanks for fixing it.


On 26/08/20 6:45 pm, Jonathan Wakely wrote:
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.



Reply via email to