Quuxplusone added inline comments.
================
Comment at: libcxx/include/memory:1645
- template <class _Tp>
+ template <class _SourceTp, class _DestTp>
_LIBCPP_INLINE_VISIBILITY
----------------
ldionne wrote:
> Coming at it from a slightly different angle, I would think this is what we
> want:
>
> ```
> template <class _SourceTp, class _DestTp,
> class _RawSourceTp = typename remove_const<_SourceTp>::type,
> class _RawDestTp = typename remove_const<_DestTp>::type>
> _LIBCPP_INLINE_VISIBILITY static typename enable_if<
> //
> We can use memcpy instead of a loop with construct if...
> is_trivially_move_constructible<_DestTp>::value && //
> - the Dest is trivially move constructible, and
> is_same<_RawSourceTp, _RawDestTp>::value && //
> - both types are the same modulo constness, and either
> (__is_default_allocator<allocator_type>::value || //
> + the allocator is the default allocator (and we know `construct` is just
> placement-new), or
> !__has_construct<allocator_type, _DestTp*, _SourceTp const&>::value), //
> + the allocator does not provide a custom `construct` method (so we'd fall
> back to placement-new)
> void>::type
> __construct_range_forward(allocator_type&, _SourceTp* __begin1, _SourceTp*
> __end1, _DestTp*& __begin2)
> {
> ptrdiff_t _Np = __end1 - __begin1;
> if (_Np > 0)
> {
> _VSTD::memcpy(const_cast<_RawDestTp*>(__begin2), __begin1, _Np *
> sizeof(_DestTp));
> __begin2 += _Np;
> }
> }
> ```
>
> And then we should have
>
> ```
> template <class _Tp>
> struct __is_default_allocator : false_type { };
>
> template <class _Tp>
> struct __is_default_allocator<_VSTD::allocator<_Tp> > : true_type { };
> ```
>
> Does this make sense?
>
> Also, I'm not sure I understand why we use `const_cast` on the destination
> type. It seems like we should instead enforce that it is non-const? But this
> is a pre-existing thing in the code, this doesn't affect this review.
>
I agree that it is wrong to express the check in terms of
`is_same<allocator_type, allocator<...>>`; it should be expressed in terms of a
trait which is satisfied by `std::allocator<T>`-for-any-T. @ldionne expressed
it in terms of `__is_default_allocator<A>`. I continue to ask that it be
expressed in terms of `__has_trivial_construct<A, _DestTp*, _SourceTp&>`, where
libc++ specializes `__has_trivial_construct<std::allocator<_Tp>, ...>` if need
be.
Orthogonally, the condition `__has_construct<allocator_type, _DestTp*,
_SourceTp const&>` is wrong because it has an extra `const`. It is conceivable
— though of course implausible/pathological — for `construct(T*, T&)` to exist
and do something different from `construct(T*, const T&)`.
================
Comment at:
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp:196
test_ctor_under_alloc();
+ test_ctor_with_different_value_type();
}
----------------
I suggest that interesting test cases include "array of `int` to vector of
`unsigned int`" (trivial, but unimplemented in this patch) and "array of
`iostream*` to vector of `ostream*`" (non-trivial because each pointer must be
adjusted).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D48342/new/
https://reviews.llvm.org/D48342
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
