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
