> On 2016-Mar-16, at 12:31, Duncan P. N. Exon Smith <dexonsm...@apple.com>
> wrote:
>
>>
>> On 2016-Mar-16, at 12:20, Eric Fiselier <e...@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`.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
