Quuxplusone added inline comments.
================
Comment at: libcxx/include/memory:1479
+struct __has_construct_missing
+ : false_type
+{
----------------
vsapsai wrote:
> 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.
I'd like to request the spelling `__has_trivial_construct_and_destroy<A, T,
T&&>` as described here: https://www.youtube.com/watch?v=MWBfmmg8-Yo&t=21m45s
I have an implementation in my fork that might be useful for comparison,
although even it doesn't add that last `T&&` parameter.
https://github.com/Quuxplusone/libcxx/commit/34eb0b5c8f03880b94d53b877cbca384783ad65a
What we're *really* interested in, in most cases, is
`__has_trivial_construct<A, T, T&&> && __has_trivial_destroy<A, T>`. We don't
care if there happens to exist an obscure overload such as `A::construct(T*,
Widget, Gadget)` as long as it is not selected. This is particularly relevant
to `scoped_allocator_adaptor`, whose `construct` is trivial for primitive types
but non-trivial for allocator-aware types.
Also, when we write out the template type parameters we care about, we can do
the decltype stuff really easily, without having to "fudge" the metaprogramming
and possibly get it wrong. For example, if `A::construct` is an overload set,
in which case the `&_Xp::construct` on this patch's line 1492 will be
ill-formed even though a non-trivial `construct` does actually exist.
https://reviews.llvm.org/D48342
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
