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 cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits