[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 requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: jkorous.

I believe this patch fixes an important QOI bug: see http://llvm.org/PR37574. I wholeheartedly agree with Eric that allocators-over-const are an abomination, however pointers-to-const are a fine thing and our implementation should handle them gracefully.

Please rebase on top of `master` -- there's another function that does not appear in this diff that should be fixed too (`__construct_forward`).



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



================
Comment at: libcxx/include/memory:1690
         <
             (is_same<allocator_type, allocator<_Tp> >::value
                 || !__has_construct<allocator_type, _Tp*, _Tp>::value) &&
----------------
This should be fixed in a similar way.


================
Comment at: libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp:186
+  std::vector<float> v(array, array + 3);
+  assert(std::fabs(v[0] - 0.0f) < FLT_EPSILON);
+  assert(std::fabs(v[1] - 1.0f) < FLT_EPSILON);
----------------
I do not understand this test, can you please explain?


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

https://reviews.llvm.org/D48342