vsapsai planned changes to this revision. vsapsai added inline comments.
================ Comment at: libcxx/include/memory:1479 +struct __has_construct_missing + : false_type +{ ---------------- vsapsai wrote: > vsapsai wrote: > > erik.pilkington wrote: > > > vsapsai wrote: > > > > 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`. > > > Oh I see, yikes! That's a pretty bad bug. I agree that this fix is best > > > then, but can you add a comment explaining this to > > > `__has_construct_missing` for future casual readers? Also, I think we > > > should move the __has_construct_missing bugfix into a different > > > (prerequisite) patch. Seems unrelated to the `const` related optimization > > > below. > > The bug as I described isn't really present now because function signature > > > > __construct_range_forward(allocator_type&, _Tp* __begin1, _Tp* __end1, > > _Tp*& __begin2) > > > > works as implicit `is_same` for `__begin1` and `__begin2` types. I think it > > is worth fixing separately and there is a bug with C++03 and custom > > allocators. > Instead of `__has_construct_missing` I've implemented real `__has_construct` > in D48753. But it is stricter in C++03 than in C++11 and later. So it made me > think that absence of `construct` with exact signature isn't a good reason to > use memcpy. I was wrong. Now I think the logic for using memcpy should be if types are the same modulo constness and ( using default allocator or using custom allocator without `construct` ) and is_trivially_move_constructible The purpose of the allocator check is to cover cases when `static construct` would end up calling not user's code but libc++ code that we know can be replaced with memcpy. https://reviews.llvm.org/D48342 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits