[PATCH] D49317: Move __construct_forward (etc.) out of std::allocator_traits.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

My review is incomplete, especially I cannot say with confidence if the proposed change is entirely free from unintended consequences that might break code not covered by the test suite. So other reviewers are welcome to chime in.



================
Comment at: include/vector:298
+__copy_construct_forward(_Alloc& __a, _Iter __begin1, _Iter __end1,
+                         _Ptr& __begin2, _CopyViaMemcpy)
+{
----------------
Why does this function use `_CopyViaMemcpy` and not `false_type` like other functions?


================
Comment at: include/vector:300
+{
+    using _Alloc_traits = allocator_traits<_Alloc>;
+    for (; __begin1 != __end1; ++__begin1, (void)++__begin2)
----------------
Have you checked why `using` is accepted in C++03 mode? The tests are passing but I expected a compiler warning and didn't investigate further.


================
Comment at: include/vector:366-367
+    ptrdiff_t _Np = __end1 - __begin1;
+    if (_Np > 0) {
+        __end2 -= _Np;
+        _VSTD::memcpy(_VSTD::__to_raw_pointer(__end2), _VSTD::__to_raw_pointer(__begin1), _Np * sizeof(_Tp));
----------------
Good. I think decrementing `__end2` after `_Np` check is better than what we had before.


================
Comment at: include/vector:542
+template<class _Tp, class _Allocator>
+struct __vector_copy_via_memcpy : integral_constant<bool,
+    (is_same<_Allocator, allocator<_Tp> >::value || !__has_construct<_Allocator, _Tp*, _Tp>::value) &&
----------------
I think the name `__vector_constructable_via_memcpy` better reflects the meaning. It detects cases when individual element construction can be safely replaced with memcpy, so it feels more about construct than about copy. And `copy_via_memcpy` is too imperative as for me, not really conveying it has boolean semantic.


================
Comment at: include/vector:1015
 {
+    typedef typename __vector_copy_via_memcpy<_Tp, _Allocator>::type __copy_via_memcpy;
     __annotate_delete();
----------------
It's not immediately obvious why there is no check like `is_same<_ForwardIterator, _Tp*>` here. My guess is that we are using variables like `this->__end_`, `v.__begin_` that we know are pointers. Don't think it's really a problem and not suggesting any changes, decided to mention it's a little bit tricky to understand.


Repository:
  rCXX libc++

https://reviews.llvm.org/D49317