[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.

[Louis Dionne via Phabricator via cfe-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/memory:1645
 
-    template <class _Tp>
+    template <class _SourceTp, class _DestTp>
         _LIBCPP_INLINE_VISIBILITY
----------------
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?



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D48342/new/

https://reviews.llvm.org/D48342