I'll upload a new patch in a moment. Replies inline below.
> On 2016-Jan-21, at 23:12, Eric Fiselier <e...@efcs.ca> wrote:
>
> 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.
When aren't they? How do they blow up?
Is there a _LIBCPP config macro we could add?
> - 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&&").
Sorry I missed this.
> 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?
Split. I'll upload the main/first part.
> I would also love if we applied this optimization to single argument
> "emplace" calls.\
Just committed r258575 which should take care of this. The new patch
will have extra tests to cover insert_hint(), emplace(), and
emplace_hint().
>
> ================
> 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.
Yes, that's better. Done.
> ================
> 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.
I'll double check this before commit (I'll look for a test and
add one if it's missing).
> ================
> 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.
Right. Fixed.
>
> ================
> 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?
Yup.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
