> 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 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
