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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.

This is very cool, thank you! I'd like to discuss (and potentially change) how tests are structured before approving.



================
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*>>>();
----------------
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.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.operations/advance.pass.cpp:23
 // Make sure we catch forced conversions to the difference_type if they happen.
-// ADDITIONAL_COMPILE_FLAGS: -Wsign-conversion
+// TODO: <ranges> includes <chrono> and <atomic> and lots of implementation headers under __chrono have signed to unsigned conversion
+// Enable the following warning once those headers have been fixed "ADDIT1IONAL_COMPILE_FLAGS_TODO: -Wsign-conversion"
----------------
Can you please provide more context here? It doesn't seem like either `<chrono>` or `<atomic>` (or `<ranges>`) are included here, and there don't seem to be any changes in includes in any of the standard headers, so why is this an issue now?

(Also: there's an extra `1` character in `ADDITIONAL` and a `_TODO` suffix which should probably be removed)


================
Comment at: libcxx/test/support/test_iterators.h:802
+// but simplified to just hold one argument.
+// Note that unlike tuple this class doesn't have special handling of swap
+template <class T>
----------------
Can you expand the comment to explain the intention here?


================
Comment at: libcxx/test/support/test_iterators.h:840
+
+  // const assignment required by indirectly_writable
+  template <class Other>
----------------
This is to make `ProxyIterator`, not `Proxy`, indirectly writable, right? If so, can you please make it clear in the comment?


================
Comment at: libcxx/test/support/test_iterators.h:860
+
+// This is to make ProxyIterator model `std::indirectly_readable`
+template <class T, class U, template <class> class TQual, template <class> class UQual>
----------------
These properties (`std::indireclty_readable`, `std::indirectly_writable`, anything else that's crucial for these classes to function correctly) should be `static_assert`ed. You can add static assertions right after the class definitions.


================
Comment at: libcxx/test/support/test_iterators.h:875
+// ======================================================================
+// It wraps `Base` iterator and when deference it returns a Proxy<ref>
+// It simulates C++23's zip_view::iterator but simplified to just wrap
----------------
Nit: `s/dereference/dereferenced/`.


================
Comment at: libcxx/test/support/test_iterators.h:878
+// one base iterator.
+// Note it forwards value_type, iter_move, iter_swap. e.g If the base
+// iterator is int*,
----------------
Nit: `s/e.g If/E.g. if/`.


================
Comment at: libcxx/test/support/test_iterators.h:889
+      typename std::iterator_traits<Base>::iterator_category,
+      std::input_iterator_tag> struct ProxyIteratorBase<Base> {
+  using iterator_category = std::input_iterator_tag;
----------------
Add line break to place `struct` on a different line.


================
Comment at: libcxx/test/support/test_iterators.h:920
+  
+  template <class T> requires std::constructible_from<Base, T&&>
+  constexpr ProxyIterator(T&& t) : base_{std::forward<T>(t)} {}
----------------
Nit: move `requires` to a different line?


================
Comment at: libcxx/test/support/test_iterators.h:1082
+    requires std::ranges::input_range<const Base>
+  {
+    return ProxyIterator{std::ranges::begin(base_)};
----------------
Ultranit: opening brace normally goes to the preceding line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129099