[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:
> 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).
Thanks for explaining. I will try to find a different approach since the implementation of copy_cvref_t seems a lot of code. One way I can think of is that instead of having this one getData template function, I can have 4 overloads, that takes &, const&, &&, const&&, so that I can specify the  cvref manually, which should be less code than copy then implementation. What do you htink


================
Comment at: libcxx/test/support/test_proxy.h:134
+
+  friend constexpr decltype(auto) base(const ProxyIterator& p) { return base(p.base_); }
+
----------------
philnik wrote:
> 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.
Hmm. I am not so familiar with the design, the reason I call the base of the base is that without doing it, this doesn't work

```
ProxyIterator<cpp20_input_iterator> == sentinel_wrapper<ProxyIterator<cpp20_input_iterator>>
```

The reason is, sentinel_wrapper stores the base of the iterator on construction, and compare it with the base of the iterator to be compared.

In this case if the base of the ProxyIterator is `cpp20_input_iterator`, the above == doesn't work because cpp20_input_iterator cannot be compared with itself.

So I one step further to get the base of the base. I think the major difference is that when we use ProxyIterator, we will have one more level nested iterators than using other test iterators

What do you think?


================
Comment at: libcxx/test/support/test_proxy.h:266
+};
+
+template <class BaseSent>
----------------
philnik wrote:
> 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).
I will try to move the code into test_iterator


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129099