ldionne var-const Mordante huixie90
- Group Reviewers
- rG7912b1f8e7c8: [libc++] Fix reverse_iterator::iterator_concept
LGTM with nits
perhaps constrain It to be at least random_access_iterator?
This is more of FYI and you don't have to follow. To define new C++20 iterators with some level of c++17 backward compatibility support, we typically define:
We actually don't want to define pointer and reference. For C++20 concepts, they are not needed, they are deduced from operator-> (if exists) and operator*. For C++17 backward compatibility, by not defining them, it will trigger the primary template iteartor_traits to properly generate them from operator* and operator-> if they exist.
By defining them it will change the rule of iterator_traits primary template.
So the above is what we typically do when writing proposals for new range adaptors
if U itself already models random_access_iterator, it is already default constructible. If not, then perhaps some of the member functions will be broken. Do we need to check here?
Again you don't have to do. A really common category of iterators that model c++20 randam_access_iterator but only c++17 input_iterator are iterators with prvalue reference type, such as iota, transform iterator with F return by value. If I wrote this class, I'd simulate that. but you don't have to
This class defined pointer to be It, but not defining operator-> , which is incorrect. I'd recommend just not define pointer since operator-> isn't required
perhaps forward iter_move and iter_swap as well in case this class is used in the future by some other tests?
perhaps also static_assert the iterator_traits::iterator_category is not derived from random access tag?
- Address comments
Thanks for the explanation!
Nope, it's just a copy-paste.
I don't know if we will ever use this with move advanced iterators. I'd rather keep it at the minimum for random_access_iterators for now. We can still update it later if we want.
I don't really see the value in that. You can just look at the typedef and know that it's the case. Verifying that random_access_iterator is satisfied is really hard OTOH.
In general looks good, but some comments.
Why does this still pass?
Either I overlook it or there are no assignment operators.
Why not use the spaceship operator for these 5 comparisons? The code is only available in C++20.
While you're at it, please add a full stop.
I guess random_access_iterator<char*> is both c++20 random_access_iterator and c++17 random_access_iterator so the test would pass anyways. the new cpp20_random_access_iterator is c++20 random_access_iterator but only c++17 input_iterator, which would fail the test if the bug weren't fixed
LGTM in essence, leaving final approval to others who chimed in on this. We should go through our archetypes and make sure they are as minimal as possible, i.e. we should probably have a cpp20_xxxxx and a cpp17_xxxxx version for each iterator that has differences between the two standards. Not for this patch, of course.
- Address comments
What exactly are you asking for? I don't think it makes a lot of sense to comment about this here, since the pattern "only C++17 input_iterator, but C++20 abc_iterator" exists everywhere in the ranges library.
There aren't iterator<U> assignment operators in any of the test iterators.
IIUC defaulting operator<=> doesn't work when the members don't have an operator<=>.