Page MenuHomePhabricator

[libc++] Fix reverse_iterator::iterator_concept
ClosedPublic

Authored by philnik on Jul 14 2022, 11:58 AM.

Diff Detail

Event Timeline

philnik created this revision.Jul 14 2022, 11:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2022, 11:58 AM
philnik requested review of this revision.Jul 14 2022, 11:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2022, 11:58 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
huixie90 accepted this revision.Jul 14 2022, 1:01 PM

LGTM with nits

libcxx/test/support/test_iterators.h
227

perhaps constrain It to be at least random_access_iterator?

235–240

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:

  • iterator_category if the class we define does model c++17 iteartor
  • iterator_concept. this is obvious
  • value_type for indirectly_readable_traits
  • difference_type for incrementable_traits

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

248

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?

253

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

309

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

311

perhaps forward iter_move and iter_swap as well in case this class is used in the future by some other tests?

316

perhaps also static_assert the iterator_traits::iterator_category is not derived from random access tag?

philnik updated this revision to Diff 445359.Jul 17 2022, 4:31 PM
philnik marked 5 inline comments as done.
  • Address comments
libcxx/test/support/test_iterators.h
235–240

Thanks for the explanation!

248

Nope, it's just a copy-paste.

311

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.

316

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.

libcxx/test/std/iterators/predef.iterators/reverse.iterators/types.compile.pass.cpp
118–119

Why does this still pass?

libcxx/test/support/test_iterators.h
244

Either I overlook it or there are no assignment operators.

292

Why not use the spaceship operator for these 5 comparisons? The code is only available in C++20.

950

While you're at it, please add a full stop.

huixie90 added inline comments.Jul 18 2022, 1:47 PM
libcxx/test/std/iterators/predef.iterators/reverse.iterators/types.compile.pass.cpp
118–119

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

Mordante added inline comments.Jul 19 2022, 1:02 PM
libcxx/test/std/iterators/predef.iterators/reverse.iterators/types.compile.pass.cpp
118–119

Thanks for the explanation @huixie90. @philnik if you agree with that assessment, can you use that as comment in the code.

ldionne accepted this revision as: ldionne.Jul 21 2022, 9:24 AM

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.

philnik updated this revision to Diff 447136.Jul 24 2022, 7:47 AM
philnik marked 6 inline comments as done.
  • Address comments
libcxx/test/std/iterators/predef.iterators/reverse.iterators/types.compile.pass.cpp
118–119

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.

libcxx/test/support/test_iterators.h
244

There aren't iterator<U> assignment operators in any of the test iterators.

292

IIUC defaulting operator<=> doesn't work when the members don't have an operator<=>.

Mordante accepted this revision.Jul 29 2022, 9:08 AM

LGTM!

This revision is now accepted and ready to land.Jul 29 2022, 9:08 AM
This revision was automatically updated to reflect the committed changes.