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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
