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

Thu Apr 7 16:49:53 PDT 2022

var-const accepted this revision as: var-const.
var-const added a comment.

LGTM once the comment about a missing test case is addressed (after which I will approve). Thanks a lot for working on this!

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122982