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