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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik marked 5 inline comments as done.
philnik added inline comments.


================
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:
> philnik wrote:
> > 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.
> I don't disagree -- I'm just wondering because it's inconsistent with what `ranges::copy` does, which is to delegate.
Yes, because `std::copy` does a few optimizations and isn't just a simple loop.


================
Comment at: libcxx/include/algorithm:60
 
   template<input_iterator I, sentinel_for<I> S, class T, class Proj = identity>
     requires indirect_binary_predicate<ranges::equal_to, projected<I, Proj>, const T*>
----------------
var-const wrote:
> Hmm, looks accidentally deleted?
Looks like it, but has nothing to do with this PR. I'll fix it.


================
Comment at: libcxx/include/algorithm:113
 
 template <class InputIterator, class Predicate>
     constexpr bool     // constexpr in C++20
----------------
var-const wrote:
> Looks accidentally deleted?
I'll upload a fix.


================
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:
> > > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122982