From: "François Dumont" <frs.dumont@gmail.com>
To: Jonathan Wakely <jwakely@redhat.com>
Cc: "libstdc++@gcc.gnu.org" <libstdc++@gcc.gnu.org>,
gcc-patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH][Hashtable 5/6] Remove H1/H2 template parameters
Date: Wed, 26 Aug 2020 22:37:03 +0200 [thread overview]
Message-ID: <d19895de-628f-7f59-4bbb-af5b0ba12417@gmail.com> (raw)
In-Reply-To: <20200826164513.GM3400@redhat.com>
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.
>
>
prev parent reply other threads:[~2020-08-26 20:37 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-17 21:31 François Dumont
2019-11-17 23:30 ` Ville Voutilainen
2019-11-18 22:10 ` François Dumont
2019-11-18 22:18 ` Ville Voutilainen
2020-07-17 10:51 ` Jonathan Wakely
2019-12-19 20:24 ` François Dumont
2020-07-17 11:35 ` Jonathan Wakely
2020-08-06 6:35 ` François Dumont
2020-08-06 9:27 ` Jonathan Wakely
2020-08-17 17:13 ` François Dumont
2020-08-25 14:30 ` Jonathan Wakely
2020-08-26 15:30 ` Jonathan Wakely
2020-08-26 15:55 ` Jonathan Wakely
2020-08-26 15:58 ` Jonathan Wakely
2020-08-26 16:45 ` Jonathan Wakely
2020-08-26 20:37 ` François Dumont [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d19895de-628f-7f59-4bbb-af5b0ba12417@gmail.com \
--to=frs.dumont@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jwakely@redhat.com \
--cc=libstdc++@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).