On Wed, Mar 16, 2016 at 1:31 PM, 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.
>
It shouldn't actually take any additional overloads. Heres the
implementation I'm hinting at.
template <class _RawValTy, class _Key, class _ValTy = typename
__unconstref<_RawValTy>::type>
struct __extract_key;
template <class _RawValTy, class _Key>
struct __extract_key<_RawValTy, _Key, _Key>; // specialization for set
template <class _RawValTy, class _Key, class _Second>
struct __extract_key<_RawValTy, _Key, pair<_Key, _Second>>; // map
specialization for non-const key.
template <class _RawValTy, class _Key, class _Second>
struct __extract_key<_RawValTy, _Key, pair<const _Key, _Second>>; // map
specialization for const key.
>
> 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.
>
Yeah, I double checked and your right.
I'll check in a "__unconstref" template within the hour. We should use that
instead of "__uncvref".
> > ================
> > 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`.
>
I disagree only because I had a hard time reading the fractured
specializations while reviewing this.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
