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

[Eric Fiselier via cfe-commits]

On Sun, Jan 24, 2016 at 9:46 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

> Great, I should have time to clean this up tomorrow afternoon.
>
> Regarding emplace, this patch as is has tests for emplace, but
> they depend on r258575 being in tree.  You asked me to revert
> that... I'll wait for your response over in that thread:
>
> http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160118/147795.html
>
> I have a few other questions inline below.
>
> > On 2016-Jan-24, at 19:25, Eric Fiselier <eric at efcs.ca> wrote:
> >
> >
> > ================
> > Comment at: include/unordered_map:401
> > @@ -400,1 +400,3 @@
> >         {return static_cast<const _Hash&>(*this)(__x.__cc.first);}
> > +#if defined(_LIBCPP_HAS_NO_RVALUE_REFERENCES)
> > +    _LIBCPP_INLINE_VISIBILITY
> > ----------------
> > Is there a reason your hiding these overloads in C++11 and beyond?
> >
> > I would prefer as few conditional compilation blocks as possible.
>
> In C++11, this code is dead and untested.  Happy to drop the
> conditionals though, it's tested in C++03 so it should be fine.
>

It's actually not dead in C++11, its just not tested. Without this overload
the hasher will still *accept* "value_type" but it performs an implicit
conversion to __hash_value_type.
This is super bad. I think __hash_value_type's ctors should be made
explicit but I'm not sure.


> On second thought, I wonder if it would be possible to skip
> this, and use the key directly like in C++11?  I'll take a
> look.
>
> > ================
> > Comment at: include/unordered_map:1004
> > @@ +1003,3 @@
> > +    _LIBCPP_INLINE_VISIBILITY
> > +    pair<iterator, bool> __insert_extract_key_if_pair(pair<_FirstTp,
> _SecondTp>&& __x)
> > +        {return __insert_extract_key<_FirstTp>(_VSTD::move(__x));}
> > ----------------
> > Overall the dispatch looks good, but it could be a lot simpler.  Instead
> of multi-level dispatch I would write a single trait to computer if "_Pp"
> is a keylike pair. Then `insert(_Pp&&)` just use that to call
> `__insert_extract_key_if_referenceable` directly using that trait.
> >
> > For example:
> > ```
> > template <class _Key, class _Pair, class _RawPair = typename
> __uncvref<_RawPair>::type>
> > struct __is_keylike_pair : false_type {};
> >
> > template <class _Key, class _Pair, class _First, class _Second>
> > struct __is_keylike_pair<_Key, _Pair, pair<_First, _Second> > :
> is_same<typename remove_const<_First>::type, _Key> {};
> > ```
>
> Above the unordered_map<> definition a good place for this, or
> would it be better to drop it in type_traits or some such?
>

Somewhere in <unordered_map> is fine.


>
> > We also should handle `pair<...>&` which you previously didn't have.
>
> I guess that got lost when I switched away from the lazy logic
> using decay.  I'll add it back.  Strange that none of the tests
> failed... I guess I'll need to find a way to cover that case.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160124/8f95c8e0/attachment-0001.html>