[libcxx-commits] [PATCH] D124328: [libc++] Forward more often to memmove in copy

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__algorithm/copy.h:51
 #endif
      )
     return std::__copy_impl<_InValueT*, _InValueT*, _OutValueT*>(__first, __last, __result);
----------------
This is one of the reasons why I like sharing implementations between ranges versions of algorithms and non-ranges versions. Even a simple algorithm like `std::copy` can become not-so-simple due to subtle reasons, and we'd risk not fixing/improving one of the two algorithms if we had two copies. Nothing to do about this comment, just a rationale for why I can appear annoying with my requests to share ranges and non-ranges code.


================
Comment at: libcxx/include/__algorithm/copy.h:59-67
+template <class>
+struct __is_trivially_copy_assignable_unwrapped_impl : false_type {};
+
+template <class _Type>
+struct __is_trivially_copy_assignable_unwrapped_impl<_Type*> : is_trivially_copy_assignable<_Type> {};
+
+template <class _Iter>
----------------
Is it possible that what you're looking for here is instead `is_contiguous_iterator<_Iter> && is_trivially_copy_assignable<value_type<_Iter>>`? I don't think we have a trait for `is_contiguous_iterator` that works with older standards and understands `__wrap_iter` properly, but we probably should if that's indeed what you mean.


================
Comment at: libcxx/include/__algorithm/copy.h:96-98
+          class = __enable_if_t<!is_copy_constructible<_InIter>::value
+                             || !is_copy_constructible<_Sent>::value
+                             || !is_copy_constructible<_OutIter>::value> >
----------------
Can you negate the condition used below instead? It makes it easier to understand what's going on:

```
__enable_if_t<!(is_copy_constructible<_InIter>::value
                && is_copy_constructible<_Sent>::value
                && is_copy_constructible<_OutIter>::value)>
```


================
Comment at: libcxx/include/__algorithm/copy.h:104-108
+template <class _InIter, class _Sent, class _OutIter>
 inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
-pair<_InIter, _OutIter> __copy(_InIter __first, _Sent __last, _OutIter __result) {
+__enable_if_t<is_copy_constructible<_InIter>::value
+           && is_copy_constructible<_Sent>::value
+           && is_copy_constructible<_OutIter>::value, pair<_InIter, _OutIter> >
----------------
Can you use `class = __enable_if_t<...>` instead to allow placing the return type in the usual place? Note that if you run into duplicate declaration issues, you can use non-type template parameters instead:

```
template <class _InIter, class _Sent, class _OutIter, __enable_if_t<CONDITION>* = nullptr>
...
```


================
Comment at: libcxx/test/libcxx/algorithms/alg.modifying.operations/copy.pass.cpp:12
+
+// <algorithm>
+
----------------
This needs a comment explaining what the test does (it tests that we optimize copy operations into `memmove`s IIUC).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124328