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

[Eric Fiselier via cfe-commits]

On Wed, Mar 16, 2016 at 1:31 PM, 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.
>

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160316/5c0fe24e/attachment.html>