[PATCH] D16360: unordered_map: Avoid unnecessary mallocs when no insert occurs

Duncan P. N. Exon Smith via cfe-commits cfe-commits at lists.llvm.org
Wed Mar 16 12:33:25 PDT 2016


> On 2016-Mar-16, at 12:31, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
>> 
>> On 2016-Mar-16, at 12:20, Eric Fiselier <eric at efcs.ca> wrote:
>> 
>> EricWF added a comment.
>> 
>> Adding inline comments for the implementation. Comments on the tests to follow shortly.
>> 
>> 
>> ================
>> Comment at: include/__hash_table:103
>> @@ -102,1 +102,3 @@
>> 
>> +template <class _ValTy, class _Key>
>> +struct __extract_key;
>> ----------------
>> Could you make `__extract_key` behave the same way as `__can_extract_key` where we apply the `__uncvref<ValTy>` instead of expecting the user to? 
> 
> I started with that, but it seemed to require many more
> explicit specializations of `__extract_key`.  It's simpler to
> handle all the possibilities via overloading.

I.e., we need the same functor whether we have:
- int
- int&
- const int
- const int&

> 
> I can have another look and see if I can find something that
> seems more symmetric, but I don't think moving the __uncref
> inside __extract_key is the right choice.
> 
>> ================
>> Comment at: include/__hash_table:110
>> @@ +109,3 @@
>> +    : is_same<
>> +          typename remove_const<typename remove_reference<_ValTy>::type>::type,
>> +          _Key> {};
>> ----------------
>> Assuming we can't be passed volatile types (I'll double check that). Then we should just use `is_same<_RawValTy, _Key>`
> 
> I think we can be passed volatile types.  `emplace` forwards all
> arguments, and the underlying type may have constructors from
> volatile types.
> 
>> ================
>> Comment at: include/__hash_table:113
>> @@ +112,3 @@
>> +template <class _Key>
>> +struct __extract_key<_Key, _Key> {
>> +  const _Key &operator()(const _Key &__key) { return __key; }
>> ----------------
>> Please keep the `__can_extract_key` and `__extract_key`  specializations together. \
> 
> I'm fine either way, but I thought it would scale better (as we
> add more of these) to have each `__can_extract_key` paired with
> its corresponding `__extract_key`.



More information about the cfe-commits mailing list