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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik requested changes to this revision.
philnik added a comment.
This revision now requires changes to proceed.

Thanks for working on this! Could you go through the list of already implemented algorithms and instantiate every one of those with at least `ProxyIterator<int*>`? If there are broken ones, please add the line commented out with a `// TODO: Fix proxy iterator` above it (to make it easier to grep for). It would also be nice if you could add `ProxyIterator<input_iterator<int*>>` through `ProxyIterator<contiguous_iterator<int*>>` to the permutation tests (since we know at least some of them are broken).



================
Comment at: libcxx/test/support/test.support/test_proxy.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
I don't object to `ProxyIterator` having it's own test, but I wouldn't ask for one either.


================
Comment at: libcxx/test/support/test.support/test_proxy.pass.cpp:17
+
+struct MoveOnly {
+  int i;
----------------
Use `MoveOnly.h`?


================
Comment at: libcxx/test/support/test_proxy.h:42
+                          Proxy> friend constexpr auto
+    getData(P&& p) -> std::__copy_cvref_t<P&&, T> {
+    return static_cast<std::__copy_cvref_t<P&&, T>>(p.data);
----------------
Please don't use implementation details. This makes our tests non-portable.


================
Comment at: libcxx/test/support/test_proxy.h:134
+
+  friend constexpr decltype(auto) base(const ProxyIterator& p) { return base(p.base_); }
+
----------------
Should this call `base` recursively?


================
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_;
+  }
----------------
Can't you just `= default` this?


================
Comment at: libcxx/test/support/test_proxy.h:266
+};
+
+template <class BaseSent>
----------------
Could you add a few `static_assert`s here to ensure that `ProxyIterator` meets the correct concepts? (That would be enough testing IMO)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129099