[libcxx-commits] [PATCH] D129099: [libcxx][ranges] Create a test tool `ProxyIterator` that customises `iter_move` and `iter_swap`

[Hui via Phabricator via libcxx-commits]

huixie90 added inline comments.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move_backward.pass.cpp:130
 
+  test_proxy_in_iterators<ProxyIterator<bidirectional_iterator<int*>>>();
+  test_proxy_in_iterators<ProxyIterator<random_access_iterator<int*>>>();
----------------
var-const wrote:
> Rather than adding these checks to every test file, would it be possible to create a dedicated test file, similar to `ranges_robust_against_copying_*` files? Happy to discuss here or on Discord if you'd like.
I also like the idea of a single file, but it is slightly trickier than `ranges_robust_against_copying_*`. The `ranges_robust_against_copying_*` test doesn't check runtime behavior. I believe for these tests, we do want to check runtime behavior. For example, for the sorting tests, we do like to check the ranges are sorted. (e.g a triple-move implementation of swap could end up with elements overwritten, and only iter_swap can correctly swap the elements).

But we definitely "can" add a "ranges_robust_against_" test just to test it compiles for all algorithms for proxy iterator. and for individual algorithms that we know they are definitely moving/swapping elements,  we can add additional tests to check that the behaviour is correct. what do you think


================
Comment at: libcxx/test/support/test_proxy.h:161-165
+  friend constexpr bool operator==(const ProxyIterator& x, const ProxyIterator& y)
+    requires std::equality_comparable<Base>
+  {
+    return x.base_ == y.base_;
+  }
----------------
philnik wrote:
> Can't you just `= default` this?
I couldn't because it has base class which don't have == defined. I think it is more effort to define the base class's operator== then just spell it out here



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129099