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