vsapsai added inline comments.
================
Comment at: libcxx/include/memory:1479
+struct __has_construct_missing
+ : false_type
+{
----------------
Quuxplusone wrote:
> 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.
I agree with benefits of using `__has_trivial_construct_and_destroy` in general
but in this case I don't see a need for `__has_trivial_destroy`.
`__construct_range_forward` only copies new elements and it isn't concerned
with destruction. Probably for some cases we can meld destroy+construct
together but I think that is out of scope for the current patch.
Arthur, can you please give some examples of possible problems with
`scoped_allocator_adaptor`. I didn't work with it closely and don't really
understand your concern.
I wish I could avoid "fudging" the metaprogramming but we need to support
C++03 and I don't see other alternatives. And in C++03 `A::construct` shouldn't
be an overload set so my understanding is that it is OK to use
`&_Xp::construct`.
https://reviews.llvm.org/D48342
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
