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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to