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

[Eric Fiselier via cfe-commits]

EricWF added a comment.

Alright, so this is looking really good.  Once we land this would you be interested in applying the same optimization to `emplace` calls?


================
Comment at: include/__hash_table:863
@@ -862,2 +862,3 @@
     _LIBCPP_INLINE_VISIBILITY
-    pair<iterator, bool> __insert_unique_value(_ValueTp&& __x);
+    pair<iterator, bool> __insert_unique_value(_ValueTp&& __x)
+        {return __insert_unique_key_value(__x, std::forward<_ValueTp>(__x));}
----------------
Woops, I know this was my suggestion, but I don't see any reason to have this function.

Please remove `__insert_unique_value`. Callers can call `__insert_unique_key_value` directly.

================
Comment at: include/__hash_table:875
@@ -867,1 +874,3 @@
+    _LIBCPP_INLINE_VISIBILITY
+    pair<iterator, bool> __insert_unique_key_value(const _ValueTp& __key, const _ValueTp& __x);
 #endif
----------------
Please use the following signature for this overload:

```
template <class _KeyLike, class _ValueTp>
pair<iterator, bool> __insert_unique_key_value(const _KeyTp& __key, const _ValueTp& __x);
```
Using `_ValueTp` twice in C++03, but `_KeyTp` in C++11 is confusing.
I understand that in C++03 `_KeyTp` should always be `_ValueTp` but it took me 10 minutes to realize that.
 

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



================
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> {};
```

We also should handle `pair<...>&` which you previously didn't have. 


http://reviews.llvm.org/D16360