[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/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);
----------------
philnik wrote:
> Please don't use implementation details. This makes our tests non-portable.
do you have any alternative suggestions for `__copy_cvref_t`? I don't want to copy the implementations here.




================
Comment at: libcxx/test/support/test_proxy.h:134
+
+  friend constexpr decltype(auto) base(const ProxyIterator& p) { return base(p.base_); }
+
----------------
philnik wrote:
> Should this call `base` recursively?
Could you please clarify? I thought my implementation does it recursively. Are you suggesting not to do it recursively?


================
Comment at: libcxx/test/support/test_proxy.h:266
+};
+
+template <class BaseSent>
----------------
philnik wrote:
> Could you add a few `static_assert`s here to ensure that `ProxyIterator` meets the correct concepts? (That would be enough testing IMO)
There are few reason I wrote the test:
1. It took me few iterations to get all the things correct and having the tests is really helpful for me to make sure the implementation is correct.

2. This header doesn't depend on those test iterators atm which makes me to put static_assert in the test. (not a big issue though)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129099