[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 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);
----------------
huixie90 wrote:
> 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.
> 
> 
I don't think you have any other option, since there are no standard copy-qualifier type traits and we haven't implemented any for the tests.
Just as a note: this isn't just a theoretical exercise. Microsoft uses the libc++ tests on their own STL implementation (i.e. to detect implementation divergence).


================
Comment at: libcxx/test/support/test_proxy.h:134
+
+  friend constexpr decltype(auto) base(const ProxyIterator& p) { return base(p.base_); }
+
----------------
huixie90 wrote:
> 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?
I'm not sure if it should be done recursively. I think we had a discussion on this at some point, but I don't remember the outcome. Looking at the `test_iterators` it looks like we don't want to call `base` recursively. I think @ldionne should know that.


================
Comment at: libcxx/test/support/test_proxy.h:266
+};
+
+template <class BaseSent>
----------------
huixie90 wrote:
> 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)
Just to make sure: I'm not saying that the test is a bad idea. I just wanted to say that I would consider the test optional.

Since you bring it up: Maybe this should just be in `test_iterators.h`. I can't really see a case where you'd want this but not the other test iterators and you probably want this in most tests where you want the test iterators (at least going forward).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129099