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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__algorithm/copy.h:47
+  if (__libcpp_is_constant_evaluated()
+// TODO: Remove this once GCC supports __builtin_memmove during constant evaluation
+#ifndef _LIBCPP_COMPILER_GCC
----------------
Do you know whether this is available in GCC-12?


================
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
----------------
philnik wrote:
> 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.
The cpp20 output iterator isn't copy constructible. But `std::basic_string<CharT>::iterator` can still be unwrapped. So I wondered whether we want an overload for that case. Something like
```
template <class _InIter, class _Sent, class _OutIter,
           __enable_if_t<is_copy_constructible<_InIter>::value
                      && is_copy_constructible<_Sent>::value> >
inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
  pair<_InIter, _OutIter> __copy(_InIter __first, _Sent __last, _OutIter __result) {
  auto __ret = std::__copy_impl(std::__unwrap_iter(__first), std::__unwrap_iter(__last), std::move__result));
  return std::make_pair(std::__rewrap_iter(__first, __ret.first), _ret.second);
}


================
Comment at: libcxx/include/__algorithm/copy_backward.h:56
-        __result -= __n;
-        _VSTD::memmove(__result, __first, __n * sizeof(_Up));
-    }
----------------
philnik wrote:
> 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`?
No the removal of this overload. Does that affect the generated code or not.


================
Comment at: libcxx/include/__algorithm/ranges_copy_if.h:49
+    }
+    return {std::move(__first), std::move(__result)};
+  }
----------------
philnik wrote:
> 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`.
I trusted the wording of the Standard, but it seems that's wrong ;-)
http://eel.is/c++draft/alg.copy#18.2
```
  {last, result + N} for the overloads in namespace ranges.
```
Do you want to file an LWG issue?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122982