[libcxx-commits] [PATCH] D130695: [libc++][ranges]Refactor `copy{, _backward}` and `move{, _backward}`

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/test/libcxx/algorithms/alg.modifying.operations/copy.pass.cpp:74
-
-  friend constexpr NotIncrementableIt operator+(const NotIncrementableIt& it, difference_type size) { return it.i + size; }
-  friend constexpr difference_type operator-(const NotIncrementableIt& x, const NotIncrementableIt& y) { return x.i - y.i; }
----------------
This test was actually incorrect -- because unwrapping iterators uses `to_address` to reduce them to pointers, the fact that this iterator is not incrementable doesn't matter -- it would work if the implementation chooses to forward to `memmove` just fine.


================
Comment at: libcxx/test/libcxx/algorithms/alg.modifying.operations/copy_move_nontrivial.pass.cpp:9
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
----------------
I think these tests can be made to work pre-C++20, but it would be painful, partially because `contiguous_iterator_tag` is a C++20 addition. Please let me know if you think it's worth the trouble.


================
Comment at: libcxx/test/libcxx/algorithms/alg.modifying.operations/copy_move_nontrivial.pass.cpp:66
+
+// Unwrapping the iterator inside `std::copy` and similar algorithms relies on `to_address`. If the `memmove`
+// optimization is used, the result of the call to `to_address` will be passed to `memmove`. This test deliberately
----------------
I have manually verified that this check works -- if I change `NonTrivialCopy`/`Move` to become trivial, this test will no longer compile, complaining that `contiguous_iterator` cannot be converted to `void*`.


================
Comment at: libcxx/test/libcxx/algorithms/alg.modifying.operations/copy_move_trivial.pass.cpp:37
+// similar algorithms.
+void* memmove(Foo* dst, Foo* src, size_t count) {
+  memmove_called = true;
----------------
I don't think it's possible to test this any other way, since it is by design that whether the optimization was applied is not observable by normal means. While it is undefined behavior to add things to the `std` namespace for user code, I would consider tests to be part of the implementation (and in any case, we control the implementation). This logic would break if the optimization changes, e.g. if the call to `memmove` were to be replaced by `__builtin_memmove`, but in that case the test will start failing, so I don't think that's a significant problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130695