[libcxx-commits] [PATCH] D122982: [libc++] Implement ranges::copy{, _n, _if, _backward}

Tue Apr 5 01:54:01 PDT 2022

philnik added inline comments.

================
Comment at: libcxx/include/__algorithm/copy.h:61
+                     && is_copy_constructible<_Sent>::value
+                     && is_copy_constructible<_OutIter>::value> >
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
----------------
Mordante wrote:
> Do we need to have an additional specialization? where
> ```
>          __enable_if_t<is_copy_constructible<_InIter>::value
>                       && is_copy_constructible<_Sent>::value
>                       && !is_copy_constructible<_OutIter>::value> >
> ```
> Just to make sure that a copy of a `std::string` to a non-copyable output iterator is still unwrapped.
> This is will be used in `std::format`.
What would you expect to change between it being wrapped and not? It's not like we can optimize that in any way.

================
Comment at: libcxx/include/__algorithm/copy.h:39-40

-template <class _InputIterator, class _OutputIterator>
-inline _LIBCPP_INLINE_VISIBILITY
-_OutputIterator
-__copy(_InputIterator __first, _InputIterator __last, _OutputIterator __result)
-{
-    return _VSTD::__copy_constexpr(__first, __last, __result);
+template <class _ValueT,
+          class = __enable_if_t<!is_const<_ValueT>::value && is_trivially_copy_assignable<_ValueT>::value> >
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
----------------
var-const wrote:
> Hmm, if I'm reading this correctly, the previous condition supported copying from `const T*` to `T*`, whereas the new condition only supports `T*` to `T*`. Am I missing something?
No, I missed something. I thought the old version said `is_same<remove_const<_Tp>::type, _Tp>`, which is obviously just `is_const`.

================
Comment at: libcxx/include/__algorithm/copy.h:51
+  if (__n > 0)
+    __builtin_memmove(__result, __first, __n * sizeof(_ValueT));
+  return pair<_ValueT*, _ValueT*>(__first + __n, __result + __n);
----------------
var-const wrote:
> Question: what's the advantage of using `__builtin_memmove` directly?
I can't use `std::memmove` in a constant expression and using `__builtin_memmove` costs a lot less steps (https://godbolt.org/z/nqb75KGa3).

================
Comment at: libcxx/include/__algorithm/copy.h:56
+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) {
----------------
var-const wrote:
> Question: what's the reason to change from `_LIBCPP_CONSTEXPR_AFTER_CXX17` to `_LIBCPP_CONSTEXPR_AFTER_CXX11`?
AFAIK the general style is to use the earliest `constexpr` possible for internal stuff.

================
Comment at: libcxx/include/__algorithm/copy.h:69
+  auto __ret = std::__copy_impl(std::__unwrap_iter(__first), std::__unwrap_iter(__last), std::__unwrap_iter(__result));
+  return pair<_InIter, _OutIter>(std::__rewrap_iter(__first, __ret.first), std::__rewrap_iter(__result, __ret.second));
 }
----------------
var-const wrote:
> Question: any reason not to use `make_pair`?
I didn't know `std::make_pair` existed. Thanks!

================
Comment at: libcxx/include/__algorithm/copy.h:76
+copy(_InputIterator __first, _InputIterator __last, _OutputIterator __result) {
+  return std::__copy(__first, __last, __result).second;
 }
----------------
var-const wrote:
> Nit: move?
This patch exists in part because we want to avoid this exact extension, so no - this should definitely not be moved. See D122074.

================
Comment at: libcxx/include/__algorithm/copy_backward.h:56
-        __result -= __n;
-        _VSTD::memmove(__result, __first, __n * sizeof(_Up));
-    }
----------------
Mordante wrote:
> Did you have a look at the codegen for this change?
What change are you asking for exactly? The change from `std::memmove` to `__builtin_memmove`?

================
Comment at: libcxx/include/__algorithm/ranges_copy_if.h:43
+  __go(_InIter __first, _Sent __last, _OutIter __result, _Pred& __pred, _Proj& __proj) {
+    for (; __first != __last; ++__first) {
+      if (std::invoke(__pred, std::invoke(__proj, *__first))) {
----------------
var-const wrote:
> Question: why doesn't this algorithm delegate to the non-ranges version (is it to avoid having to deal with the projection)?
Why would it? It's a really simple algorithm and has no pitfalls AFAIK.

================
Comment at: libcxx/include/__algorithm/ranges_copy_if.h:49
+    }
+    return {std::move(__first), std::move(__result)};
+  }
----------------
Mordante wrote:
> The original is correct, but this is closer to the wording of the Standard.
The problem is that your version isn't correct. `_Sent` doesn't have to be the same type as `_InIter`.

================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_backward.pass.cpp:50
+template <class In, class Out, class Sent = In>
+constexpr void test_iterators() {
+  { // simple test
----------------
var-const wrote:
> I think the current tests always use trivially copyable values -- can we add at least one test case where values are not trivially copyable?
`CopyOnce` and `OnlyBackwardsCopyable` aren't trivial.

================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_backward.pass.cpp:154
+    struct OnlyBackwardsCopyable {
+      OnlyBackwardsCopyable* next = nullptr;
+      bool canCopy = false;
----------------
var-const wrote:
> Optional: `s/next/prev/`?
I thought of it as 'the next copyable value'. I'd rather keep it as-is.

================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_if.pass.cpp:115
+      std::array in = {4, 6, 87, 3};
+      std::array<int, 2> out;
+      auto ret = std::ranges::copy_if(In(in.data()),
----------------
var-const wrote:
> I'd suggest making this array as large as `in` and relying on the return value to make sure only two elements are copied. Otherwise, it's not clear if the implementation would stop at two elements if the output array was larger.
I'd argue that it should be exact. `std::ranges::copy_if` can't know the size of the range it gets passed, and if it would try to write to more elements than are available this will error during constant evaluation. i.e. an implementation could `*(out + 1) = *in` and you suggestion wouldn't catch that, while making the array wit the exact size would.

================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_if.pass.cpp:150
+constexpr bool test() {
+  test_in_iterators<cpp17_output_iterator<int*>>();
+  test_in_iterators<forward_iterator<int*>>();
----------------
Mordante wrote:
> I would like to see tests for `cpp20_output_iterator`.
I'll do that as soon as you land D122072.

================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_n.pass.cpp:98
+
+  return true;
+}
----------------
var-const wrote:
> Question: should this algorithm also check the order in which the elements are copied? (same with `copy_if.pass.cpp`)
for `copy_if` and `copy_n` it isn't specified in which order the elements are copied.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122982