Quuxplusone added inline comments.

================
Comment at: include/vector:298
+__copy_construct_forward(_Alloc& __a, _Iter __begin1, _Iter __end1,
+                         _Ptr& __begin2, _CopyViaMemcpy)
+{
----------------
vsapsai wrote:
> Why does this function use `_CopyViaMemcpy` and not `false_type` like other 
> functions?
Oops, that's totally cruft left over from an earlier revision. Fixed!


================
Comment at: include/vector:300
+{
+    using _Alloc_traits = allocator_traits<_Alloc>;
+    for (; __begin1 != __end1; ++__begin1, (void)++__begin2)
----------------
vsapsai wrote:
> 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.
I talked with Glen Fernandes about this on Slack the other day. I think the 
deal is that `make check-cxx` runs only the `-std=c++2a` tests, and if you want 
`-std=c++03` you have to run them manually with `llvm-lit --param=-std=c++03 
-sv path/to/tests`. Which of course I didn't do. :)
If there's a more foolproof way of automatically testing libc++ in *all* 
compiler modes, I'd like to know about it.

Fixed!


================
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) &&
----------------
vsapsai wrote:
> 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.
> `copy_via_memcpy` is too imperative for me

I see your point. However, for background... in my other branch, this trait is 
joined by two companions:
```
struct __vector_relocate_via_memcpy
struct __vector_destroy_via_noop
```
So I'd like a naming scheme that fits all three use-cases comfortably.

How about just adding the word "should"?  
`__vector_should_construct_via_memcpy`, `__vector_should_destroy_via_noop`, 
etc?  Would that sufficiently address the "too imperative" issue?


================
Comment at: include/vector:1015
 {
+    typedef typename __vector_copy_via_memcpy<_Tp, _Allocator>::type 
__copy_via_memcpy;
     __annotate_delete();
----------------
vsapsai wrote:
> 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.
Your guess is 100% correct, AFAIK. All we're doing here is copying from one 
`__split_buffer` to another, so both sides are always a contiguous range.


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