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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik 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
----------------
Mordante wrote:
> Do you know whether this is available in GCC-12?
It's not available in GCC trunk.


================
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:
> 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);
> }
I think the unwrapping is mostly so that we can do a `memmove` if we have only raw pointers. I don't think that the unwrapping of only part of the iterators makes any difference. The compiler should still be able to see through the wrapper and optimize it away.


================
Comment at: libcxx/include/__algorithm/copy_backward.h:56
-        __result -= __n;
-        _VSTD::memmove(__result, __first, __n * sizeof(_Up));
-    }
----------------
Mordante wrote:
> 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.
The codegen is a bit worse, but I doubt it would even be measurable. It's two instructions more. https://godbolt.org/z/Pb7YbnsK3


================
Comment at: libcxx/include/__algorithm/ranges_copy_if.h:49
+    }
+    return {std::move(__first), std::move(__result)};
+  }
----------------
Mordante wrote:
> 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?
I'm not sure if this is an oversight or if this is expected. Most ranges algorithms return `last` according to the standard, while in practice they have to return `first + N`. It's the same with `N` itself. The standard says `last - first`, while in practice it would have to be `std::distance(first, last)`.


================
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:
> philnik wrote:
> > var-const wrote:
> > > philnik wrote:
> > > > 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.
> > > Yes, but those tests are focused on checking something else. I'd prefer to have a dedicated test for this, even if it's somewhat redundant with the other tests.
> > What exactly do you want to check just with a non-trivially copyable type? If we do nothing inside the copy constructor/assignment operator there is nothing to check. The implementation could just as well `memcpy` the whole thing.
> Right, this is one thing to check -- that the implementation doesn't try to `memcpy` non-trivial types, for example. But really, all I want to check is that the result is correct, not for any particular kind of problem because too many are possible to consider. Just the fact that it would produce the correct result without crashing is a valuable thing to test, even if it appears trivial. Furthermore, tests are written for the future as well. Even if an algorithm is "obviously" trivial and seemingly cannot be incorrect now, it could change in the future.
> 
I don't disagree that it's a valuable test. My problem is that the test you propose is a strict subset of what `// check that the range is copied backwards` tests IIUC. I don't think the test adds any coverage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122982