<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Jan 24, 2016 at 9:46 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Great, I should have time to clean this up tomorrow afternoon.<br>
<br>
Regarding emplace, this patch as is has tests for emplace, but<br>
they depend on r258575 being in tree.  You asked me to revert<br>
that... I'll wait for your response over in that thread:<br>
<a href="http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160118/147795.html" rel="noreferrer" target="_blank">http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160118/147795.html</a><br>
<br>
I have a few other questions inline below.<br>
<span class=""><br>
> On 2016-Jan-24, at 19:25, Eric Fiselier <<a href="mailto:eric@efcs.ca">eric@efcs.ca</a>> wrote:<br>
><br>
><br>
> ================<br>
> Comment at: include/unordered_map:401<br>
> @@ -400,1 +400,3 @@<br>
>         {return static_cast<const _Hash&>(*this)(__x.__cc.first);}<br>
> +#if defined(_LIBCPP_HAS_NO_RVALUE_REFERENCES)<br>
> +    _LIBCPP_INLINE_VISIBILITY<br>
> ----------------<br>
> Is there a reason your hiding these overloads in C++11 and beyond?<br>
><br>
> I would prefer as few conditional compilation blocks as possible.<br>
<br>
</span>In C++11, this code is dead and untested.  Happy to drop the<br>
conditionals though, it's tested in C++03 so it should be fine.<br></blockquote><div><br></div><div>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.</div><div>This is super bad. I think __hash_value_type's ctors should be made explicit but I'm not sure.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On second thought, I wonder if it would be possible to skip<br>
this, and use the key directly like in C++11?  I'll take a<br>
look.<br>
<span class=""><br>
> ================<br>
> Comment at: include/unordered_map:1004<br>
> @@ +1003,3 @@<br>
> +    _LIBCPP_INLINE_VISIBILITY<br>
> +    pair<iterator, bool> __insert_extract_key_if_pair(pair<_FirstTp, _SecondTp>&& __x)<br>
> +        {return __insert_extract_key<_FirstTp>(_VSTD::move(__x));}<br>
> ----------------<br>
> 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.<br>
><br>
> For example:<br>
> ```<br>
> template <class _Key, class _Pair, class _RawPair = typename __uncvref<_RawPair>::type><br>
> struct __is_keylike_pair : false_type {};<br>
><br>
> template <class _Key, class _Pair, class _First, class _Second><br>
> struct __is_keylike_pair<_Key, _Pair, pair<_First, _Second> > : is_same<typename remove_const<_First>::type, _Key> {};<br>
> ```<br>
<br>
</span>Above the unordered_map<> definition a good place for this, or<br>
would it be better to drop it in type_traits or some such?<br></blockquote><div><br></div><div>Somewhere in <unordered_map> is fine.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> We also should handle `pair<...>&` which you previously didn't have.<br>
<br>
</span>I guess that got lost when I switched away from the lazy logic<br>
using decay.  I'll add it back.  Strange that none of the tests<br>
failed... I guess I'll need to find a way to cover that case.</blockquote></div><br></div></div>