Quuxplusone added inline comments.
================
Comment at: libcxx/include/memory:1645
- template <class _Tp>
+ template <class _SourceTp, class _DestTp>
_LIBCPP_INLINE_VISIBILITY
----------------
ldionne wrote:
> Quuxplusone wrote:
> > 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&)`.
> > 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.
>
> Would you be OK with us applying this fix and then generalizing
> `__is_default_allocator` into `__has_trivial_construct` as a followup? I
> suspect we'll have more discussion around that generalization and I'd like
> for us to fix this bug because I find PR37574 somewhat concerning and I'd
> like for it to be fixed soon (like within a couple of days).
>
> > 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&)`.
>
>
> Good catch. IIUC, `__has_construct<allocator_type, _DestTp*, _SourceTp&>`
> would work?
>
> Would you be OK with us applying this fix and then generalizing
> `__is_default_allocator` into `__has_trivial_construct` as a followup?
Yes, I would; although I am somewhat cynical about the followup ever actually
happening. :P
> IIUC, `__has_construct<allocator_type, _DestTp*, _SourceTp&>` would work?
Yes.
================
Comment at: libcxx/include/memory:1658
+ || !__has_construct<allocator_type, _DestTp*, const
_SourceTp&>::value) &&
+ is_trivially_move_constructible<_DestTp>::value,
void
----------------
Shouldn't this be something like `is_trivially_constructible<_DestTp,
_SourceTp&>`? I mean, we're never doing move-construction here. We're
constructing `_DestTp` from `_SourceTp&`, which is either copy-construction or
non-const-copy-construction, depending on the constness of `_SourceTp`.
`is_trivially_constructible` has pitfalls in general —
https://quuxplusone.github.io/blog/2018/07/03/trivially-constructible-from/ —
but I think we won't fall into any of those pits as long as we're testing that
`_SourceTp` is cv-qualified `_DestTp`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D48342/new/
https://reviews.llvm.org/D48342
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
