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

[Eric Fiselier via cfe-commits]

EricWF added a comment.

Overall the patch looks good but I have a few concerns.

> - If argument.first can be trivially converted to key_type, don't alloc.


I'm concerned with this part of the change because:

- The `is_trivially_*` traits are often not available and can sometimes blow up.
- It also makes the implementation of `__insert_extract_key_if_pair` a lot more complicated.
- It is technically non-standard because the standard says "insert(P&&)` should be handled as-if "emplace(P&&").

I would like to put a lot more thought into this part of your patch. Could you remove it and add it in a follow up?

I would also love if we applied this optimization to single argument "emplace" calls.


================
Comment at: include/__hash_table:867
@@ -864,1 +866,3 @@
+    _LIBCPP_INLINE_VISIBILITY
+    pair<iterator, bool> __insert_unique_key_value(const _KeyTp &__key, _ValueTp&& __x);
 #else
----------------
I really don't like how the name and signature of the function changes between C++03 and C++11. It's confusing me a lot while reviewing the patch.  I would much rather it always accept two parameters and be called `__insert_unique_key_value`. 

We can add a C++03 __insert_unique_value that dispatches like you do above if needed.

================
Comment at: include/unordered_map:403
@@ +402,3 @@
+    _LIBCPP_INLINE_VISIBILITY
+    size_t operator()(const typename _Cp::value_type& __x) const
+        {return static_cast<const _Hash&>(*this)(__x.first);}
----------------
This part LGTM so long as instantiating _Cp in the function signature doesn't prevent unordered_map from being used with incomplete types.

================
Comment at: include/unordered_map:993
@@ +992,3 @@
+                _VSTD::forward<_Pp>(__x),
+                is_same<typename decay<_FirstTp>::type, key_type>());
+        }
----------------
We don't want to decay because we don't want "array-to-pointer" and "function-to-pointer" conversions.  We just want to remove the cv and ref qualifiers.

I *just* added a "__uncvref"trait in <type_traits>. Please use that instead.

On second thought, I don't think we want to remove the volatile qualifiers.

================
Comment at: include/unordered_map:998
@@ +997,3 @@
+    _LIBCPP_INLINE_VISIBILITY
+    pair<iterator, bool> __insert_extract_key_if_referenceable(_Pp&& __x, false_type)
+        {
----------------
I'm pretty concerned about the trivially_constructible part of the patch/optimization.  Can we handle this in a follow up revision instead?



http://reviews.llvm.org/D16360