[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai planned changes to this revision.
vsapsai added inline comments.


================
Comment at: libcxx/include/memory:1479
+struct __has_construct_missing
+    : false_type
+{
----------------
erik.pilkington wrote:
> Shouldn't this be true_type?
I see this is confusing and I'm still struggling how to express it. The issue is that in C++03 `__has_construct` should be something like unknown, so that neither `__has_construct` nor `! __has_construct` evaluate to true because we don't really know if allocator has construct. This case is covered by the added test, in C++03 the memcpy specialization was enabled when

```
    is_same<allocator_type, allocator<_Tp> >
      || !false_type
```

So `is_same` check had no effect and we were using memcpy to convert between int and float.

I was considering using something like

```lang=c++
        typename enable_if
        <
            (is_same
             <
                typename _VSTD::remove_const<typename allocator_type::value_type>::type,
                typename _VSTD::remove_const<_SourceTp>::type
             >::value
#ifndef _LIBCPP_CXX03_LANG
                || !__has_construct<allocator_type, _DestTp*, _SourceTp>::value
#endif
            ) &&
             is_trivially_move_constructible<_DestTp>::value,
            void
        >::type
```

But that doesn't look readable to me, so I've introduced ad-hoc ternary logic with `__has_construct_missing`.


================
Comment at: libcxx/include/memory:1673-1677
+            (is_same
+             <
+                typename _VSTD::remove_const<typename allocator_type::value_type>::type,
+                typename _VSTD::remove_const<_SourceTp>::type
+             >::value
----------------
erik.pilkington wrote:
> I'm not sure if this is correct. Previously, we only selected this overload if the allocator didn't have a construct() member, or if it was a std::allocator, in which case we know construct() just called in-place new. With this patch, we would select this overload for a custom allocator that overrode construct() so long as the value_types matched. I think the right behaviour for the custom allocator case would be to use the construct() member instead of memcpying.
Thanks for pointing out the case with custom allocator. My expectation is that it should use construct() instead of memcpy, I agree with you. Will check further and plan to add a test.


https://reviews.llvm.org/D48342