[libc++][ranges] Implement changes to reverse_iterator from One Ranges Proposal.

[var-const]

Summary

Changes in P0896:

• add disable_sized_sentinel_for;
• add iter_move and iter_swap;
• add a requires clause to the operator->;
• add iterator_concept;
• check that the Iterator template parameter is a bidirectional iterator;
• add constraints to all comparison operators;
• change the definitions of iterator_category, value_type, difference_type and reference (changes to iterator_category were already implemented).

Also add a few forgotten things to the reverse_iterator synopsis
(notably the spaceship operator).

[var-const]

Use the actual patch number.

[var-const]

Remove forgotten extraneous test.

[var-const]

Here are some changes which I think don't affect the implementation, but please double-check my logic:

1. Removed the part that said

"The fundamental relation between a reverse iterator and its corresponding iterator i is established by the identity: &*(reverse_iterator(i)) == &*(i - 1).

2. Previously it was said that "Iterator shall meet the requirements of a Cpp17RandomAccessIterator" if certain operators are "referenced in a way that requires instantiation". Now it just says "instantiated".

3. Previously, operator-> was described as "Returns: addressof(operator*())". Now the description says:

Effects:

• if Iterator is a pointer type, equivalent to: return prev(current);
• otherwise, equivalent to: return prev(current).operator->();

It seems like the current implementation of operator-> should satisfy this, but please let me know if it's wrong.

libcxx/include/__iterator/iterator_traits.h

145

This is a drive-by change: I just found the original comment unclear.

libcxx/include/__iterator/reverse_iterator.h

53

http://eel.is/c++draft/reverse.iterators#reverse.iter.requirements-1:

"The template parameter Iterator shall either meet the requirements of a Cpp17BidirectionalIterator ([bidirectional.iterators]) or model bidirectional_­iterator ([iterator.concept.bidir])."

My understanding is that this is IFNDR, but adding a check seems like it could prevent harder-to-read errors during instantiation.

There's a similar requirement for random access iterators:

"Additionally, Iterator shall either meet the requirements of a Cpp17RandomAccessIterator ([random.access.iterators]) or model random_­access_­iterator ([iterator.concept.random.access]) if the definitions of any of the members operator+, operator-, operator+=, operator-= ([reverse.iter.nav]), or operator[] ([reverse.iter.elem]), or the non-member operators ([reverse.iter.cmp]) operator<, operator>, operator<=, operator>=, operator-, or operator+ ([reverse.iter.nonmember]) are instantiated ([temp.inst])."

I'm not sure that can be done with a static_assert -- I tried placing it into the bodies of the mentioned operators, and the compiler produces errors trying to instantiate non-existing functions before it evaluates the static_assert.

71

Note: this change to iterator_category was done previously for all language modes, even though it only applies to C++20 and later.

libcxx/include/iterator

230

Some of these changes might be a bit pedantic -- please let me know.

256–263

From what I could see, the return type was unspecified all the way back in C++03, so this looks like it was always incorrect.

262

The Standard's formatting is a bit peculiar here.

272

Use the same order as the Standard.

libcxx/test/std/iterators/predef.iterators/reverse.iterators/iterator_concept_conformance.compile.pass.cpp

21

These don't work anymore because there is no operator== to compare I1::base() == std::reverse_iterator<float*>::base(). Previously, trying to compare the two would fail upon instantiation, but this check didn't trigger instantiation, so it could succeed (though arguably it shouldn't have).

If you have ideas on how to replace these checks, please let me know.

[• Quuxplusone]

Partially reviewed. I cut out in the middle of iter_move; I assume my comment there also applies to iter_swap.

libcxx/include/__iterator/iterator_traits.h

145

Sure. FWIW, I'd just remove this comment. It kinda seems like the only point of this comment is to explain something that the comment itself says is "obvious," which means it's like self-referentially bad. Plus, I don't even know what it's talking about. If "the Cpp17*Iterator tables" make an appearance in libc++'s codebase, I don't know where.

Perhaps all this comment means is: "__iterator_traits_detail::__cpp17_iterator is not the same as __is_cpp17_input_iterator." But that really is obvious, and only serves to raise more questions, like "Wait, why isn't it?" So again, I think the solution is to delete the confusing comment.

libcxx/include/__iterator/reverse_iterator.h

53

clang-format strikes again, in the formatting here; please fix it up somehow.
I'm ambivalent on adding this static_assert. It will probably worsen the error messages, actually, since "such-and-such operator doesn't exist when I tried to use it" is generally much easier to deal with than "this random static_assert was false instead of true." Have you actually looked at the error messages for e.g. reverse_iterator<std::forward_list<int>::iterator>, and do you think this is an improvement?
Here's the state of the art, for comparison: https://godbolt.org/z/nne1Mvzan

__iterator/reverse_iterator.h:120:37: error: cannot decrement value of type 'std::__forward_list_iterator<std::__forward_list_node<int, void *> *>'
    reverse_iterator& operator++() {--current; return *this;}
                                    ^ ~~~~~~~

I predict you'll find that this static_assert is not an improvement, and decide not to keep it.

71

Right, we want to do it in all language modes, just in case someone ever tries to use some Boost contiguous_iterator tag in C++17 mode or something.

77

Please group these blocks together; see what I did in https://reviews.llvm.org/D117656#change-18S54d7VLfJo
I also recommend getting rid of the cute space-alignment formatting as long as you're in the area.

148–153

I think what I'd do here is

#if _LIBCPP_STD_VER > 17
    pointer operator->() const
#if !defined(_LIBCPP_HAS_NO_CONCEPTS)
      requires is_pointer_v<_Iter> || requires(const _Iter i) { i.operator->(); }
#endif // !defined(_LIBCPP_HAS_NO_CONCEPTS)
    {
      _Iter __tmp = current;
      --__tmp;
      if constexpr (is_pointer_v<_Iter>) {
        return __tmp;
      } else {
        return _VSTD::move(__tmp).operator->();
      }
    }
#else
    pointer operator->() const {
      return _VSTD::addressof(operator*());
    }
#endif // _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_CONCEPTS)

Drive-by removed the cute spacing in pointer operator->.

162

Either don't mess with the spacing at all, or fully update it as shown. (I have a slight preference for "don't mess with," but either is OK.)

173–179

clang-format strikes again. Ditto iter_swap.

libcxx/include/iterator

230

FWIW, I'll never care much about synopsis comments. However, it seems like you've got several (trivial?) copy-paste errors.

232

238

libcxx/test/libcxx/iterators/predef.iterators/reverse.iterators/bad_template_argument.fail.cpp

22–23 ↗

(On Diff #410075)

I believe you wanted this to be .verify.cpp, not .fail.cpp. I think (but am not sure) that .fail.cpp just checks for any compiler failure, and doesn't actually respect //expected-error comments.

However, I expect this to be moot when you decide to remove the static_assert again. :)

libcxx/test/std/iterators/predef.iterators/reverse.iterators/iterator_concept_conformance.compile.pass.cpp

21

This is one of these awful metaprogrammed tests. I don't even know what I1 is, here, so I don't see why it should be a sentinel for reverse_iterator<float*> in particular. Deleting these lines seems great to me (and if you want to rewrite this whole test, that would be even greater ;))

To replicate the possible intent of these lines, you might add at the bottom of this test file

static_assert( std::equality_comparable_with<std::reverse_iterator<int*>, std::reverse_iterator<const int*>>);
static_assert(!std::equality_comparable_with<std::reverse_iterator<int*>, std::reverse_iterator<char*>>);
static_assert( std::totally_ordered_with<std::reverse_iterator<int*>, std::reverse_iterator<const int*>>);
static_assert(!std::totally_ordered_with<std::reverse_iterator<int*>, std::reverse_iterator<char*>>);
static_assert( std::three_way_comparable_with<std::reverse_iterator<int*>, std::reverse_iterator<const int*>>);
static_assert(!std::three_way_comparable_with<std::reverse_iterator<int*>, std::reverse_iterator<char*>>);

57–59

These three lines don't seem to add anything, but it would be nice to have a test for some type B such that bidirectional_iterator<B> && !random_access_iterator<B> && sized_sentinel_for<B, B>. Unfortunately I think we don't have such a type in test_iterators.h, but it might take only ~15 lines to write one here (especially since you don't have to implement any of its methods, just declare them).

I also think you should actually split these lines out into reverse.iterators/sized_sentinel.compile.pass.cpp, and add another test for a type R such that essentially-random_access_iterator<R> && disable_sized_sentinel_for<R, R>, and make sure that requires { reverse_iterator<R>() - reverse_iterator<R>(); } == false.

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/sfinae.compile.pass.cpp

68–78

Since you're in C++20-only and have concepts, please

template<class T> concept HasEqualTo = requires (T t) { t == t; };
template<class T> concept HasNotEqual = requires (T t) { t != t; };
template<class T> concept HasLess = requires (T t) { t < t; };
template<class T> concept HasLessEqual = requires (T t) { t <= t; };
template<class T> concept HasGreater = requires (T t) { t > t; };
template<class T> concept HasGreaterEqual = requires (T t) { t >= t; };
template<class T> concept HasSpaceship = requires (T t) { t <=> t; };

and then use those throughout. And you might as well test all 7 concepts for each of your types.

You know I'm not thrilled with : IterBase, but it seems to be saving a lot of lines of code, so I won't complain (much) this time. ;) However, doesn't this all run afoul of your static_assert, too? It's not legal to create the type reverse_iterator<NoEqualityCompIter> for at least two reasons: an iterator must be comparable to itself, and an iterator must also have operator++, which none of these do. But you needn't do anything about this yet... just wait for @CaseyCarter to tell us that it makes MSVC's implementation unhappy. ;)

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.elem/arrow.pass.cpp

79–81

Can you find a way to use concept/requires in this file too? This one is harder because it's not C++20-and-concepts-only, but maybe with some TEST_STD_VER ifdefs...? I also notice the @Mordante-irking use of _LIBCPP_HAS_NO_CONCEPTS below; and the gratuitous drive-by reformatting of main. I think we could kill all three birds with one stone if you just split out these new tests into arrow.sfinae.pass.cpp (UNSUPPORTED: c++03, c++11, c++14, c++17; UNSUPPORTED: libcpp-has-no-concepts), and then don't touch arrow.pass.cpp at all. Does that work for you?

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/iter_move.pass.cpp

26–33

Since this test is C++20-only, you don't actually need the old-school Big Five. You might not even need any of them, except probably difference_type because you have no operator-. But you definitely don't need pointer or reference, and I think not even iterator_category.

[var-const]

Address feedback and rebase.

libcxx/include/__iterator/iterator_traits.h

145

I agree that the comment is still vague and confusing; it presumes the reader has a lot of context and leaves that context out. I think the most confusing part is "Cpp17*Iterator tables" (which is also the part you pointed out). Most likely, it refers to [iterator.cpp17](https://eel.is/c++draft/iterator.cpp17), which indeed contains some tables (though mentioning them here is at best unhelpful). I tweaked the wording a bit -- do you think it reads better now?

libcxx/include/__iterator/reverse_iterator.h

53

I'm actually formatting most of these manually (don't want to get used to the convenience of clang-format when there's a chance it's taken away :) ). Reformatted this bit with clang-format now, I think it's an improvement.

Here's the full error I get with a static assert upon trying to create a reverse iterator from a forward iterator:

In file included from /Users/varconst/work/llvm-project/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/ctor.default.pass.cpp:15:
In file included from /Users/varconst/work/llvm-project/build/include/c++/v1/iterator:669:
In file included from /Users/varconst/work/llvm-project/build/include/c++/v1/__iterator/reverse_access.h:14:
/Users/varconst/work/llvm-project/build/include/c++/v1/__iterator/reverse_iterator.h:49:5: error: static_assert failed due to requirement '__is_cpp17_bidirectional_iterator<forward_iterator<int *>>::value || bidirectional_iterator<forward_iterator<int *>>' "The Iterator template argument must be a bidirectional iterator."
    static_assert(__is_cpp17_bidirectional_iterator<_Iter>::value || bidirectional_iterator<_Iter>,
    ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/varconst/work/llvm-project/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/ctor.default.pass.cpp:40:13: note: in instantiation of template class 'std::reverse_iterator<forward_iterator<int *>>' requested here
    BadIter i;
            ^
1 error generated.

Compared to the baseline (where construction succeeds but e.g. trying to increment the iterator fails):

In file included from /Users/varconst/work/llvm-project/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/ctor.default.pass.cpp:15:
In file included from /Users/varconst/work/llvm-project/build/include/c++/v1/iterator:669:
In file included from /Users/varconst/work/llvm-project/build/include/c++/v1/__iterator/reverse_access.h:14:
/Users/varconst/work/llvm-project/build/include/c++/v1/__iterator/reverse_iterator.h:148:37: error: cannot decrement value of type 'forward_iterator<int *>'
    reverse_iterator& operator++() {--current; return *this;}
                                    ^ ~~~~~~~
/Users/varconst/work/llvm-project/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/ctor.default.pass.cpp:42:5: note: in instantiation of member function 'std::reverse_iterator<forward_iterator<int *>>::operator++' requested here
    ++i;
    ^
1 error generated.

For easier comparison, the same messages with all the cruft removed:

__iterator/reverse_iterator.h:49:5: error: static_assert failed due to requirement '__is_cpp17_bidirectional_iterator<forward_iterator<int *>>::value || bidirectional_iterator<forward_iterator<int *>>' "The Iterator template argument must be a bidirectional iterator."
    static_assert(__is_cpp17_bidirectional_iterator<_Iter>::value || bidirectional_iterator<_Iter>,
    ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

__iterator/reverse_iterator.h:148:37: error: cannot decrement value of type 'forward_iterator<int *>'
    reverse_iterator& operator++() {--current; return *this;}
                                    ^ ~~~~~~~

I do think that the static assertion improves the error message slightly. The error is caught early and the message explains the fundamental problem rather than a symptom of the problem. Error messages like the current one are often cited as one of the problems with C++ templates -- showing a technical detail in the instantiation. While it's not hard to figure out why a reverse iterator needs the decrement operator, it's still an additional mental step, and I think saying that the iterator has to be bidirectional is a better way to express the requirement than dealing with individual requirements of a bidirectional iterator.

77

PTAL -- is this what you had in mind?

148–153

Done (with very slight modification).

Do you think it's worth thinking whether it's possible to test for the change in the effects of this function in C++20?

162

Updated.

173–179

Nope, I simply prefer this formatting. Changed.

libcxx/include/iterator

232

Thanks!

libcxx/test/libcxx/iterators/predef.iterators/reverse.iterators/bad_template_argument.fail.cpp

22–23 ↗

(On Diff #410075)

I just checked and it behaves exactly the same (correctly) with either name. So probably the implementations of verify and fail converged at some point?

libcxx/test/std/iterators/predef.iterators/reverse.iterators/iterator_concept_conformance.compile.pass.cpp

21

Thanks!

57–59

These three lines don't seem to add anything

The main intention for these lines is to provide more context for the actual test rather than test anything on their own. Basically, "if the underlying iterators are sized, sized sentinel for reverse_iterator works; otherwise, it doesn't". They also verify that the reason sized_sentinel_for is false is because of the underlying iterators, not because it's written in such a way that it's always false.

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/sfinae.compile.pass.cpp

68–78

Thanks for the suggestion, this looks much better now.

It's not legal to create the type reverse_iterator<NoEqualityCompIter>

Yes, I think our definition of __is_cpp17_bidirectional_iterator is pretty lax.

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.elem/arrow.pass.cpp

79–81

I like the idea of splitting this into its own file.

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/iter_move.pass.cpp

26–33

reverse_iterator uses the typedefs from iterator_traits for its base class (from which it still inherits even in C++20 and above if _LIBCPP_ABI_VERSION is less than 2 -- when compiling the tests locally, it is 1. iterator_traits require either all 5 to be defined (with the exception of pointer in C++20) or for the iterator to satisfy C++17Iterator (which seems like it won't be much of a win). I got rid of pointer, though it doesn't change things all that much.

[• Quuxplusone]

libcxx/include/__iterator/iterator_traits.h

145

It's better now, but I would still prefer if it were just gone.

libcxx/include/__iterator/reverse_iterator.h

53

Hm! I was hoping you'd quickly agree that

--current;
^ ~~~~~~~

was obviously the better error message.

static_assert(__is_cpp17_bidirectional_iterator<_Iter>::value || bidirectional_iterator<_Iter>,
^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

sadly gives the programmer no information about how to fix the problem with their code. So that makes it a worse error message.

I'll pull rank on this one. :) Please remove the static_assert.

148–153

Yes, you certainly could write a test for it, so sure, let's do it!

struct It {
    [...all the usual members...]
    int& operator*() const; // not defined
    int *operator->() const&; // not defined
    int *operator->() && { return nullptr; }
};
std::reverse_iterator<It> r;
assert(r.operator->() == nullptr);

173–179

FWIW, I have no strong preference myself on whether the arguments should be vertically aligned or (2|4)-space-indented. But I do have a strong preference that when breaking a binary operation, the operator/separator should always be attached to the line above, not the line below:

int a = x +
  y;  // good
int b = x
  + y;  // bad

(with libc++'s style for X() : a_() being a notable exception, and a ? b : c ambivalent)

libcxx/test/std/iterators/predef.iterators/reverse.iterators/iterator_concept_conformance.compile.pass.cpp

57–59

I'd still like you to create a reverse.iterators/sized_sentinel.compile.pass.cpp with the two tests I suggest in the parent comment. We need to test the interaction with disable_sized_sentinel_for and the effect on the reverse_iterator's own operator-.

63–64

If this line is too long, consider (1) renaming unsized_iterator to It, (2) moving the tests into a (never-called) void test() { purely so that you can use nice short { } scopes for these typedef names.

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/sfinae.compile.pass.cpp

45

I don't think you use test_iterators.h in this file.

63

What about operator<=>?

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.elem/arrow.sfinae.compile.pass.cpp

24–25

Nit.

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/iter_move.pass.cpp

26–33

Works fine for me with only value_type and difference_type.
https://godbolt.org/z/3Mv3bnn9W
I am 90% confident that you should eliminate this base class and just copy those two member typedefs into the 4 places you need them.
Ditto in iter_swap.pass.cpp.

[var-const]

Address feedback

libcxx/include/__iterator/iterator_traits.h

145

I think it would only make sense to remove the comment if it were obvious from a glance. I find the situation not obvious -- somewhat confusing, actually -- so personally I find the comment helpful. If you have any other suggestions on how to improve the wording, please let me know.

libcxx/include/__iterator/reverse_iterator.h

53

I think both are workable, so I'm ok with removing the static_assert, but I still feel that saying why it doesn't compile is more useful than saying what doesn't compile.

sadly gives the programmer no information about how to fix the problem with their code

It leads the programmer in the right direction -- they can look up the bidirectional_iterator requirements or just do static_assert(std::bidirectional_iterator<MySupposedlyBidirectionalIterator>) to see what exactly is missing from their type (which might be more than just the operator--).

[var-const]

libcxx/include/__iterator/reverse_iterator.h

148–153

Done, thanks! (I've had to provide definitions for some of the unimportant functions -- otherwise I'm getting errors like

function 'main(int, char **)::It::operator*' has internal linkage but is not defined [-Werror,-Wundefined-internal]

).

173–179

FWIW, I prefer the opposite -- I find it easier when for each subexpression, the operand is right in front of it so I don't have to scan the previous lines (of varying length) to find it. It also makes it easier to insert lines or move them around because this way, they are more "self-contained".

libcxx/test/std/iterators/predef.iterators/reverse.iterators/iterator_concept_conformance.compile.pass.cpp

57–59

Moved the tests into a new file.

add another test for a type R such that essentially-random_access_iterator<R> && disable_sized_sentinel_for<R, R>, and make sure that requires { reverse_iterator<R>() - reverse_iterator<R>(); } == false.

I'm probably missing something, but reverse_iterator::operator- doesn't check for sized_sentinel_for directly, instead relying on whether operator- is defined for the underlying iterators. It seems like this test would check the behavior of the underlying iterators.

63–64

Renamed to {,un}sized_it (but personally I think that trying to avoid line breaks isn't really worth it).

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/sfinae.compile.pass.cpp

63

My intention was to check the constraints section of these operators ("Constraints: x.base() <= y.base() is well-formed and convertible to bool"). operator<=> has no constraints section -- unless it should be inferred implicitly somehow?

By the way -- do you think it's worthwhile checking the return type as well? Basically, run all the same checks if the return type is ConvertibleToBool or NotConvertibleToBool? It seems like overkill, but happy to add the tests if you think it's worth testing.

[var-const]

Clarify comment.

[• Quuxplusone]

libcxx/include/__iterator/reverse_iterator.h

173–179

easier to insert lines or move them around

It occurs to me that I'd definitely break std::cout << "x" << "y" between "x" and << "y" (although in that case I'd also insert a new ; std::cout so that the statement didn't need to span lines in the first place).
For other operations like x && y or x + y, which don't tend to be chained extensively or ever have new operands inserted in the middle, [insert what I said before].
Btw, I'm a big fan of "trailing comma" in enum definitions and array initializers, and I wish a trailing comma were syntactically allowed everywhere (e.g. function parameter lists, member-initializer-lists).

enum E {
    A,
    B,
};

There we get the best of both worlds IMO: the comma gets to stay at the end of the line and we can easily insert new lines in the middle or at the end.

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/iter_move.pass.cpp

30

I don't think you ever use the fact that adl::Iterator is a template. Please remove the templateness.

82–85

No big deal; but I prefer the shorter code with fewer identifiers on my mental "stack". Otherwise I'm looking and waiting for the place where N will be used, and it never comes.

Ditto throughout.

90–94

I suggest removing lines 89-93. We don't need to test operator++ here.

138

What's the -- doing here and on lines 115 and 159? You can remove it, right?
...er, actually, I guess the long names confused me. This line isn't testing anything about reverse_iterator, but merely about the underlying iterator type NoexceptCopyThrowingMove (which I would just call It in each case).
So lines 136-137 are just testing the setup, and then line 139 is the important line.
Personally I'd still remove lines 136-137, because line 139 is clear enough: we're testing that iter_move is noexcept iff its dependencies are noexcept.

IOW, I'd make this

{
  struct It {
    using value_type = int;
    using difference_type = ptrdiff_t;
    explicit It() noexcept;
    It(const It&) noexcept;
    int& operator*() const;  // if you need to define a body here, OK
    It& operator++() noexcept;
    It operator++(int) noexcept;
    It& operator--() noexcept;
    It operator--(int) noexcept;
  };
  std::reverse_iterator<It> ri;
  ASSERT_NOT_NOEXCEPT(iter_move(ri));  // because operator*
}

Oh hey, in replacing the name NoexceptCopyThrowingMove with It, I discovered that the old name was wrong! Type It has no move-ctor at all, much less a throwing one. The non-noexcept operations on lines 121-139 were operator* and operator-- (I assume one or the other was accidental). So the type could have been named NoexceptCopyThrowingDeref or something like that... but fortunately now I don't have to come up with a name for it, because It is fine and good. :)

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/iter_swap.pass.cpp

32

Ditto here: I don't think you use the fact that adl::Iterator is a template.

libcxx/test/std/iterators/predef.iterators/reverse.iterators/types.pass.cpp

0

You know I'm never thrilled by tests that customize bits of types they don't own. Consider defining struct FooIter as its own type.

15–24

Is the purpose of this test just to make sure that reverse_iterator<BarIter> takes its difference_type from iterator_traits<BarIter>::difference_type and not from BarIter::difference_type directly? ...But wait, BarIter::difference_type doesn't exist! And it wouldn't exist for an iterator type like int*, either. So I actually don't think this test is adding any coverage, and therefore would prefer to remove it. Unless I'm missing something.

Which I must be, because how on earth do we end up with is_same_v<typename std::reverse_iterator<BarIter>::reference, bool&> on line 112??

[var-const]

Address review feedback.

[var-const]

libcxx/include/__iterator/reverse_iterator.h

173–179

(I made the change, by the way)

I see your point, but I still find it easier to have a single rule that covers everything.

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/iter_move.pass.cpp

82–85

Unless you feel strongly, I'd rather keep as is. For me, the constant a) underscores that the exact value is unimportant; b) makes it obvious that this is the length of the array, as opposed to some arbitrary index within the array; c) prevents me from having to keep track whether the number is the same throughout the scope (with a constant, there would be a compiler error if it's mistyped).

138

What's the -- doing here and on lines 115 and 159?

I'm emulating how the noexcept specification is defined in iter_move:

noexcept(is_nothrow_copy_constructible_v<_Iter> &&
    noexcept(ranges::iter_move(--declval<_Iter&>()))
)

I discovered that the old name was wrong! Type It has no move-ctor at all, much less a throwing one.

You're right, thanks a lot for noticing this -- this is a mistake on my part. The name should replace Move with Decrement.

I think, however, that this is evidence in favor of keeping both the verbose (but descriptive) names and verifying the properties of the dependencies. Together, they add a certain redundancy (in the information-theory sense) that makes it easier to recover from mistakes. Imagine, for the sake of the argument, that it was the class implementation that was wrong (possibly due to copy-pasting), not the name. It would be much more difficult to notice with a name like It. When the name, the implementation and checking the properties reinforce each other, it's easier to see what exactly is being tested and whether it works like expected.

libcxx/test/std/iterators/predef.iterators/reverse.iterators/types.pass.cpp

0

I understand your concern, but I strongly dislike increasing the amount of boilerplate. I would agree if this were a more "arbitrary" type, but our test iterators have well-defined semantics and quite a few dependencies, so it seems reasonable to rely on this class being there and not changing radically. So I'd rather keep as is, unless you feel strongly about it.

15–24

Is the purpose of this test just to make sure that reverse_iterator<BarIter> takes its difference_type from iterator_traits<BarIter>::difference_type..?

BarIter is only for testing reference (to make sure it's implemented correctly both pre- and post-C++20). difference_type is tested via FooIter.

how on earth do we end up with is_same_v<typename std::reverse_iterator<BarIter>::reference, bool&> on line 112?

Pre-C++20, reference would be defined as iterator_traits<I>::reference, so it's taken from the specialization of iterator_traits. In C++20+, reference is iter_reference_t, which is defined as decltype(*declval<T&>());. So the type is taken from the return type of BarIter::operator*() (see line 82).

[• Quuxplusone]

I'm inclined to green-light this at this point. I haven't given it the fine-tooth-comb treatment this time, but it's been through enough rounds of review and I assume nothing that we covered earlier has regressed lately. However:

• CI is red for apparently multiple reasons; please make sure it becomes green.
• I'd like a second libc++ reviewer to take a look too.

Happy to take another look if anything major comes up in the second person's review.

libcxx/include/__iterator/reverse_iterator.h

77

Close. I'd definitely move iterator_concept down to coalesce those two _LIBCPP_STD_VER > 17 blocks together. I'd also either duplicate the iterator_category and pointer lines, or move them up next to iterator_type, to keep the non-ifdef'ed stuff in a block together.

148–153

I finally notice your slight modification, and I'm not thrilled by it. Let's not break apart

_LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
pointer operator->() const

from the function's curly-braced body just to save two lines of duplication. We generally try (and everyone should generally try ;)) to put preprocessor conditionals only around "sensible" units of code — like, (an extreme example,) prefer

#if X
    return x+1;
#else
    return x;
#endif

over

    return x
#if X
        +1
#endif
        ;

175–176

184–186

Btw, the difference between

noexcept(ranges::iter_swap(--declval<_Iter&>(), --declval<_Iter2&>()))

and

noexcept(ranges::iter_swap(--__x, --__y))

is subtle. Do you have a test that will fail if someone makes that substitution? (If not, please add one.)

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/ctor.default.pass.cpp

39 ↗

(On Diff #412959)

This is the only diff in this test file; consider reverting it.

libcxx/test/std/iterators/predef.iterators/reverse.iterators/sized_sentinel.compile.pass.cpp

26

I don't know if they're supposed to work in C++20, but in practice, compilers don't like non-dependent requires clauses. https://reviews.llvm.org/harbormaster/unit/view/2833598/ You'll have to rewrite this kind of thing as

template<class T> concept HasMinus = requires (T t) { t - t; };

static_assert(!HasMinus<std::reverse_iterator<unsized_it>>);

which is honestly easier on the eyes anyway. :)

Circa line 20 above, you should also assert that HasMinus<std::reverse_iterator<sized_it>> (even though I guess it's obvious... well, just for symmetry, let's assert it anyway).

libcxx/test/std/iterators/predef.iterators/reverse.iterators/types.pass.cpp

15–24

Btw, I believe my confusion at first was due mainly to thinking FooIter and BarIter were the same type (i.e. I misread Bar as Foo or vice versa). I see where bool comes from, now.

It strikes me that FooIter is just plain old bidirectional_iterator — er, kinda sorta — whenever it is not true that TEST_STD_VER > 17. So really, FooIter is a "C++20-only test case"; you dutifully assert things about it when <= 17, but those things aren't really interesting things, because half of the type is missing. I think the entire definition of FooIter should be guarded by TEST_STD_VER > 17.

As I type that comment (specifically the "kinda sorta" part), I recall why inheriting from test iterators is a bad idea: you'll find that std::bidirectional_iterator<FooIter> == false, am I right? In fact FooIter isn't any kind of iterator in C++20, right? This doesn't render the test UB, but it does mean that you're not actually saving too much boilerplate here — you can omit all the iterator boilerplate! I bet you don't need much more than

#if TEST_STD_VER > 17
struct FooIter {
    using value_type = void*;
    using difference_type = void*;
};
template <>
struct std::indirectly_readable_traits<FooIter> {
  using value_type = int;
};
template <>
struct std::incrementable_traits<FooIter> {
  using difference_type = char;
};

static_assert(std::is_same_v<typename std::reverse_iterator<FooIter>::value_type, int>);
static_assert(std::is_same_v<typename std::reverse_iterator<FooIter>::difference_type, char>);
#endif // TEST_STD_VER > 17

The BarIter test should remain enabled for all C++ versions, i.e. something like

struct BarIter {
  bool& operator*() const;
};
template <>
struct std::iterator_traits<BarIter> {
  using difference_type = char;
  using value_type = char;
  using pointer = char*;
  using reference = char&;
  using iterator_category = std::bidirectional_iterator_tag;
};

#if TEST_STD_VER > 17
static_assert(std::is_same_v<typename std::reverse_iterator<BarIter>::reference, bool&>);
#else
static_assert(std::is_same_v<typename std::reverse_iterator<BarIter>::reference, char&>);
#endif

[var-const]

• address feedback;
• fix CI;
• rebase on main;
• fix names of the helper structs in iter_swap.pass.cpp.

libcxx/include/__iterator/reverse_iterator.h

77

Done (moved up the common lines to avoid duplication).

148–153

Ok, I reverted to your original version with another slight modification -- duplicated _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14 between the two clauses as well (which seems to be consistent with your comment -- I think you originally omitted it for brevity, right?).

184–186

Hmm, but __x and __y are const references, so calling -- on them should be invalid, or at least unreasonable? Also, trying to make the change as-is (without adding a new test), I get:

In file included from libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/iter_swap.pass.cpp:20:
In file included from build/include/c++/v1/iterator:642:
In file included from build/include/c++/v1/__iterator/common_iterator.h:18:
build/include/c++/v1/__iterator/iter_swap.h:41:7: error: exception specification of 'iter_swap<int *>' uses itself
      iter_swap(_VSTD::forward<_T1>(__x), _VSTD::forward<_T2>(__y));
      ^
build/include/c++/v1/__iterator/iter_swap.h:41:7: note: in instantiation of exception specification for 'iter_swap<int *>' requested here
build/include/c++/v1/__iterator/iter_swap.h:41:7: note: in instantiation of requirement here
      iter_swap(_VSTD::forward<_T1>(__x), _VSTD::forward<_T2>(__y));
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
build/include/c++/v1/__iterator/iter_swap.h:40:5: note: while substituting template arguments into constraint expression here
    requires (_T1&& __x, _T2&& __y) {
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
build/include/c++/v1/__iterator/iter_swap.h:51:16: note: while checking the satisfaction of concept '__unqualified_iter_swap<const std::reverse_iterator<int *> &, const std::reverse_iterator<int *> &>' requested here
      requires __unqualified_iter_swap<_T1, _T2>
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
build/include/c++/v1/__iterator/iter_swap.h:51:16: note: while substituting template arguments into constraint expression here
      requires __unqualified_iter_swap<_T1, _T2>
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
build/include/c++/v1/__iterator/reverse_iterator.h:190:20: note: while checking constraint satisfaction for template 'operator()<const std::reverse_iterator<int *> &, const std::reverse_iterator<int *> &>' required here
          noexcept(ranges::iter_swap(__x, __y))) {
                   ^~~~~~
build/include/c++/v1/__iterator/reverse_iterator.h:190:20: note: in instantiation of function template specialization 'std::ranges::__iter_swap::__fn::operator()<const std::reverse_iterator<int *> &, const std::reverse_it
erator<int *> &>' requested here
libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/iter_swap.pass.cpp:76:41: note: in instantiation of exception specification for 'iter_swap<int *>' requested here
    static_assert(std::same_as<decltype(iter_swap(rb, re)), void>);
                                        ^

Do you think it's worth digging further, given this?

libcxx/test/std/iterators/predef.iterators/reverse.iterators/types.pass.cpp

15–24

Done.

I still need to define the classic 5 types because reverse_iterator inherits from iterator_traits (evidently _LIBCPP_ABI_NO_ITERATOR_BASES is false when running tests, in my environment at least). And operator* for good measure (because of how reference is defined).

I think the issue with inheritance that you mention can be solved via CRTP.

[var-const]

@Quuxplusone Thanks a lot for a very thorough review! I really appreciate all the feedback.

[ldionne]

Looks really good, thanks! I have some comments so I'm requesting changes, but there's nothing major.

libcxx/include/__iterator/reverse_iterator.h

53

I have to say -- I think the static_assert approach is superior for a couple of reasons (in no order):

• Instantiating reverse_iterator with a non-bidi iterator is a precondition type of error, per http://eel.is/c++draft/reverse.iterators#reverse.iter.requirements-1. What we normally do for these errors is assert, or static_assert when detectable at compile-time. So there's a consistency argument.
• A static_assert firing makes it clear that the user messed up. When a user sees a static_assert with a message we bothered writing for them, they immediately go "oh, I must have messed up", which provides a nicer experience than letting them scratch their head over "why is this code not compiling".
• We have a message we can use to make it arbitrarily clear what's going wrong. We shouldn't be shy to explain the issue properly so as to make the static_assert undoubtedly easier to understand than the compilation error.
• Without a static_assert, it's possible that some users would try to fix their code by just adding the missing operator to their iterator type, without necessarily making it a proper bidirectional iterator. So they'd still be in IFNDR-land, when we could instead have helped them understand the problem and fix their code.
• It is generally better to fail earlier rather than later. For example, we don't want users to use reverse_iterator<non-bidi> successfully until they finally try to use operator++ on it, which would start failing. For instance, without the assertion, I think users could use reverse_iterator<non-bidi> successfully if they only used operator--, right? If so, that's not a good thing.

By the way, the above applies to anywhere in the library where we have ill-formed code that we're able to diagnose. If we agree on this, we can take it as a blanket policy to prefer static_assert over "lazy instantiation errors" when we have those cases.

So, all that being said, my vote would go for adding a static_assert with a nice error message, and adding a libc++ specific test to check that we do catch it.

62

The intent of this change is so that a reverse_iterator<contiguous-iterator> advertises itself as a random_access_iterator, not a contiguous_iterator. Right?

136

This can be made unconditionally constexpr now.

138

You should be able to replace all instances of _LIBCPP_HAS_NO_CONCEPTS by _LIBCPP_STD_VER > 17 now that we've bumped the compilers.

For this specific instance, it means it can be eliminated altogether.

142–145

Any reason for not using verbatim what's in the standard, i.e. prev(current)?

154–191

Suggestion: land these NFCs in a separate commit, no need to review or anything. That'll clean up this diff.

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/iter_move.pass.cpp

65

Is there a reason why this isn't in a constexpr function and then called as test(); static_assert(test(), ""); so that we test it both at runtime and at compile-time?

110

Thanks for being diligent on testing even the noexcept clauses. I'm glad this is the standard we hold ourselves to, even though I know those tests are tedious to write.

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/iter_swap.pass.cpp

66

The same question/comment about constexpr testing applies here too.

libcxx/test/std/iterators/predef.iterators/reverse.iterators/types.pass.cpp

42

Suggestion: move this test to a .compile.pass.cpp test and rename main to something else to remove the impression that we're running anything.

[var-const]

Address feedback.

[var-const]

libcxx/include/__iterator/reverse_iterator.h

53

Restored the static_assertion and its test (bad_template_argument.verify.cpp in libcxx tests). Please let me know if you'd like to expand the error message.

One thing to mention, though -- the Standard's constraints are:

The template parameter Iterator shall either meet the requirements of a Cpp17BidirectionalIterator ([bidirectional.iterators]) or model bidirectional_­iterator ([iterator.concept.bidir]).

I presumed that __is_cpp17_bidirectional_iterator implements Cpp17BidirectionalIterator. However, it seems significantly more lax -- if I'm reading the implementation correctly, it only checks the iterator_category, not the provided operations. So the current check is significantly weaker than it should be -- any type providing the expected iterator_category and the other 3-4 typedefs would do. Do you think I should fix this in this patch or a follow-up? Or perhaps create a local version of __is_cpp17_bidirectional_iterator in this file?

For instance, without the assertion, I think users could use reverse_iterator<non-bidi> successfully if they only used operator--, right? If so, that's not a good thing.

I tried it out to verify, and yes, functions like operator-- and base do work if there is no static_assertion.

62

I think so (it definitely has this effect), but I would have to do some archaeology to find out if that was the main intention (the One Ranges Proposal omits any rationale). Please let me know if you'd like me to confirm.

138

That's really cool, done.

142–145

I don't think there was a particularly strong reason. Changed to the Standard version.

[• Quuxplusone]

libcxx/include/__iterator/reverse_iterator.h

53

FWIW, I still strongly disagree with this direction, on principle; but will defer to @ldionne.

Without a static_assert, [...] just adding the missing operator to their iterator [...] they'd still be in IFNDR-land

True, if your goal is to keep people out of IFNDR-land entirely, then the static_assert is the right answer. I still think that for anything in "STL Classic", outside of namespace ranges, it would be better to keep the "Classic" wild-west template semantics so as to avoid breaking code unnecessarily... Basically, I like IFNDR-land. :)

the current check is significantly weaker than it should be

FWIW IMHO, it's not worth creating a local version of anything for just this file; either strengthen __is_cpp17_bidirectional_iterator or do nothing. But if strengthening __is_cpp17_bidirectional_iterator, I'd again caution to avoid breaking existing code if possible. (Or at least avoid breaking it without good reason. For @ldionne, "staying out of IFNDR-land" might count as a good reason. For me it wouldn't. So that's why we end up on different sides of this issue.)

Notice that your new tests are solidly in IFNDR-land right now; Ctrl+F down for where you said "__is_cpp17_bidirectional_iterator is pretty lax." Are you volunteering yourself to clean up that test code now? ;)

62

I'd explain it as: We know a reverse_iterator is always at least a bidirectional iterator. But if the _Iter type is random-access, then our reverse_iterator should also be random-access!

Your explanation adds: ''...But if the _Iter type is contiguous, then... nothing special. Just leave the reverse_iterator as random-access in that case." Which is absolutely correct, but it's kind of the boring part of the explanation, IMHO. ;)

142–145

@var-const: AFAIK, this should be std::prev, not ranges::prev. Please use std::prev. I'm not sure the difference is detectable, but if you can think of some way it is detectable then please add a regression test too.

184–186

Do you think it's worth digging further, given this?

For the record, no, that's fine. :)

libcxx/test/libcxx/iterators/predef.iterators/reverse.iterators/bad_template_argument.fail.cpp

22–23 ↗

(On Diff #410075)

Aha, see libcxx/utils/libcxx/test/format.py:

FOO.verify.cpp          - Compiles with clang-verify. This type of test is
                          automatically marked as UNSUPPORTED if the compiler
                          does not support Clang-verify.

FOO.fail.cpp            - Compiled with clang-verify if clang-verify is
                          supported, and equivalent to a .compile.fail.cpp
                          test otherwise. This is supported only for backwards
                          compatibility with the test suite.

So, there's no difference, but for consistency please rename to .verify.cpp.

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/sfinae.compile.pass.cpp

63

operator<=> has no constraints section -- unless it should be inferred implicitly somehow?

It has a requires-clause which expresses the same constraints. (A requires-clause makes it a constrained template, i.e., a template with constraints. :) The Standard just doesn't need to say the constraints in English because they're already said in code.)

run all the same checks if the return type is ConvertibleToBool or NotConvertibleToBool?

Hmm, so it would be something like this instead?

struct B { explicit operator bool() const; };
struct NB { };

template<int Version>
struct NoEqualityCompIter : IterBase {
  NB operator==(NoEqualityCompIter) const requires (Version == 1);
  B operator!=(NoEqualityCompIter) const;
  B operator<(NoEqualityCompIter) const;
  B operator>(NoEqualityCompIter) const;
  B operator<=(NoEqualityCompIter) const;
  B operator>=(NoEqualityCompIter) const;
};

static_assert( HasEqual<std::reverse_iterator<int*>>);
static_assert(!HasEqual<std::reverse_iterator<NoEqualityCompIter<1>>>);
static_assert( HasNotEqual<std::reverse_iterator<NoEqualityCompIter<1>>>);
static_assert( HasLess<std::reverse_iterator<NoEqualityCompIter<1>>>);
static_assert( HasLessOrEqual<std::reverse_iterator<NoEqualityCompIter<1>>>);
static_assert( HasGreater<std::reverse_iterator<NoEqualityCompIter<1>>>);
static_assert(!HasEqual<std::reverse_iterator<NoEqualityCompIter<2>>>);
static_assert( HasNotEqual<std::reverse_iterator<NoEqualityCompIter<2>>>);
static_assert( HasLess<std::reverse_iterator<NoEqualityCompIter<2>>>);
static_assert( HasLessOrEqual<std::reverse_iterator<NoEqualityCompIter<2>>>);
static_assert( HasGreater<std::reverse_iterator<NoEqualityCompIter<2>>>);

That's not terrible but also not thrilling. I'm weakly against, but will leave it up to you and/or whoever has a stronger opinion.

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

57–61

This metaprogramming was a yellow flag for me, especially after all our discussion about lax iterator requirements for legacy iterators. Could you take a look at this Godbolt and see if you agree that it's conforming C++ (i.e. It does literally satisfy the legacy requirements so we're not in IFNDR-land)? Does this PR make us pass this test case? Could you make sure this is tested somewhere?
https://godbolt.org/z/zsfYseKK6

[var-const]

Address feedback.

libcxx/include/__iterator/reverse_iterator.h

53

Notice that your new tests are solidly in IFNDR-land right now

Strengthening the requirement by removing the __is_cpp17_bidirectional_iterator part (so that it's just bidirectional_iterator), only 4 tests fail out of 36. I think the cleanup shouldn't be too time consuming if it becomes necessary.

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/sfinae.compile.pass.cpp

63

Added some tests for operator<=>.

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

57–61

It doesn't satisfy the requirement that the iterator given to reverse_iterator is at least a bidirectional iterator.

[ldionne]

Thanks! LGTM with rewording suggestion for diagnostic.

libcxx/include/__iterator/reverse_iterator.h

54

I'd prefer something like reverse_iterator<It> requires It to be a bidirectional iterator.

[var-const]

• Address feedback;
• rebase on main and fix the operator-> SFINAE test now that test_iterators.h types don't provide operator->.

[var-const]

Fix CI.

[smeenai]

libcxx/include/__iterator/reverse_iterator.h

53

I'm wading into this debate a year later :) I'm in the process of upgrading our libc++-using codebase to C++20, and I ran into this static_assert from https://github.com/facebook/folly/blob/8c52d79a616f7a89095d3121a7f02394c16c0848/folly/DynamicConverter.h#L88-L95 being instantiated with std::unordered_map. In the context of template metaprogramming, what's the best way to detect that a reverse_iterator can be constructed from an iterator before attempting to construct it, to avoid the static_assert? I could duplicate the check in the static_assert itself, of course, but I assume __is_cpp17_bidirectional_iterator isn't something I should be using outside the library, and just checking the other condition might be too restrictive.

[philnik]

libcxx/include/__iterator/reverse_iterator.h

53

You can make your T a bidirectional_iterator. I think all C++17 bidirectional iterators should meet the requirements.

[smeenai]

libcxx/include/__iterator/reverse_iterator.h

53

The T in question here (that's being passed to reverse_iterator) is std::unordered_map::iterator, so it's out of my control. That's why I was looking for the right way to verify that a reverse_iterator could be formed before attempting to form it (which would trigger the static_assert).

libcxx/docs/Status/RangesPaper.csv

 `[std.iterator.tags] <https://wg21.link/std.iterator.tags>`_,"| `contiguous_iterator_tag <https://llvm.org/rG45d048c20440989df2b4e1be1f9343225e7741ab>`_
 | `iterator_concept specialization for pointers <https://llvm.org/rG45d048c20440989df2b4e1be1f9343225e7741ab>`_
 ",[iterator.traits],Eric Fiselier,✅
 `[range.iter.ops] <https://wg21.link/range.iter.ops>`_,"| `ranges::advance <https://llvm.org/D101922>`_
 | `ranges::distance <https://llvm.org/D102789>`_
 | `ranges::next <https://llvm.org/D102563>`_
 | `ranges::prev <https://llvm.org/D102564>`_",[iterator.concepts],Christopher Di Bella and Arthur O'Dwyer,✅
 `[predef.iterators] <https://wg21.link/predef.iterators>`_,"
-| Updates to reverse_iterator
+| `Updates to reverse_iterator <https://llvm.org/D120180>`_
 | `Updates to back_insert_iterator <https://llvm.org/D103273>`_
 | `Updates to front_insert_iterator <https://llvm.org/D103273>`_
 | `Updates to move_iterator <https://llvm.org/D117656>`_","| [iterator.concepts]
 | [iterator.cust.swap]
 | [iterator.cust.move]",Various,In progress
 `[move.sentinel] <https://wg21.link/move.sentinel>`_,`move_sentinel <https://llvm.org/D117656>`_,[predef.iterators],Arthur O'Dwyer,In progress
 `[common.iterator] <https://wg21.link/common.iterator>`_,`common_iterator <https://llvm.org/D103335>`_,"| [iterator.concepts]
 | [iterator.cust.swap]

libcxx/include/__iterator/iterator_traits.h

private:
     template <class _Up> static __two __test(...);
     template <class _Up> static char __test(typename _Up::iterator_concept* = nullptr);
 public:
     static const bool value = sizeof(__test<_Tp>(nullptr)) == 1;
 };
 
 #if _LIBCPP_STD_VER > 17
 
-// The `cpp17-*-iterator` exposition-only concepts are easily confused with the Cpp17*Iterator tables,
-// so they've been banished to a namespace that makes it obvious they have a niche use-case.
+// The `cpp17-*-iterator` exposition-only concepts have very similar names to the `Cpp17*Iterator` named requirements
+// from `[iterator.cpp17]`. To avoid confusion between the two, the exposition-only concepts have been banished to
+// a "detail" namespace indicating they have a niche use-case.
 namespace __iterator_traits_detail {
 template<class _Ip>
 concept __cpp17_iterator =
   requires(_Ip __i) {
     {   *__i } -> __can_reference;
     {  ++__i } -> same_as<_Ip&>;
     { *__i++ } -> __can_reference;
   } &&

libcxx/include/__iterator/reverse_iterator.h

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP___ITERATOR_REVERSE_ITERATOR_H
 #define _LIBCPP___ITERATOR_REVERSE_ITERATOR_H
 
 #include <__compare/compare_three_way_result.h>
 #include <__compare/three_way_comparable.h>
+#include <__concepts/convertible_to.h>
 #include <__config>
+#include <__iterator/concepts.h>
+#include <__iterator/incrementable_traits.h>
+#include <__iterator/iter_move.h>
+#include <__iterator/iter_swap.h>
 #include <__iterator/iterator.h>
 #include <__iterator/iterator_traits.h>
+#include <__iterator/prev.h>
+#include <__iterator/readable_traits.h>
 #include <__memory/addressof.h>
+#include <__utility/move.h>
 #include <type_traits>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 

 #endif
 {
 _LIBCPP_SUPPRESS_DEPRECATED_POP
 private:
 #ifndef _LIBCPP_ABI_NO_ITERATOR_BASES
     _Iter __t; // no longer used as of LWG #2360, not removed due to ABI break
 #endif
 
+#if _LIBCPP_STD_VER > 17
+    static_assert(__is_cpp17_bidirectional_iterator<_Iter>::value || bidirectional_iterator<_Iter>,
+        "reverse_iterator<It> requires It to be a bidirectional iterator.");
+#endif // _LIBCPP_STD_VER > 17
+
 protected:
     _Iter current;
 public:
-    typedef _Iter                                            iterator_type;
-    typedef typename iterator_traits<_Iter>::difference_type difference_type;
-    typedef typename iterator_traits<_Iter>::reference       reference;
-    typedef typename iterator_traits<_Iter>::pointer         pointer;
-    typedef _If<__is_cpp17_random_access_iterator<_Iter>::value,
-        random_access_iterator_tag,
-        typename iterator_traits<_Iter>::iterator_category>  iterator_category;
-    typedef typename iterator_traits<_Iter>::value_type      value_type;
+    using iterator_type = _Iter;
 
+    using iterator_category = _If<__is_cpp17_random_access_iterator<_Iter>::value,
+                                  random_access_iterator_tag,
+                                  typename iterator_traits<_Iter>::iterator_category>;
+    using pointer = typename iterator_traits<_Iter>::pointer;
 #if _LIBCPP_STD_VER > 17
-    typedef _If<__is_cpp17_random_access_iterator<_Iter>::value,
+    using iterator_concept = _If<__is_cpp17_random_access_iterator<_Iter>::value,
                                   random_access_iterator_tag,
-        bidirectional_iterator_tag>                          iterator_concept;
+                                  bidirectional_iterator_tag>;
+    using value_type = iter_value_t<_Iter>;
+    using difference_type = iter_difference_t<_Iter>;
+    using reference = iter_reference_t<_Iter>;
+#else
+    using value_type = typename iterator_traits<_Iter>::value_type;
+    using difference_type = typename iterator_traits<_Iter>::difference_type;
+    using reference = typename iterator_traits<_Iter>::reference;
 #endif
 
 #ifndef _LIBCPP_ABI_NO_ITERATOR_BASES
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
     reverse_iterator() : __t(), current() {}
 
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
     explicit reverse_iterator(_Iter __x) : __t(__x), current(__x) {}
 
reverse_iterator& operator=(const reverse_iterator<_Up>& __u) {
         current = __u.base();
         return *this;
     }
 #endif
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
     _Iter base() const {return current;}
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
     reference operator*() const {_Iter __tmp = current; return *--__tmp;}
+
+#if _LIBCPP_STD_VER > 17
+    _LIBCPP_INLINE_VISIBILITY
+    constexpr pointer operator->() const
+      requires is_pointer_v<_Iter> || requires(const _Iter i) { i.operator->(); }
+    {
+      if constexpr (is_pointer_v<_Iter>) {
+        return std::prev(current);
+      } else {
+        return std::prev(current).operator->();
+      }
+    }
+#else
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
-    pointer  operator->() const {return _VSTD::addressof(operator*());}
+    pointer operator->() const {
+      return std::addressof(operator*());
+    }
+#endif // _LIBCPP_STD_VER > 17
+
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
     reverse_iterator& operator++() {--current; return *this;}
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
-    reverse_iterator  operator++(int) {reverse_iterator __tmp(*this); --current; return __tmp;}
+    reverse_iterator operator++(int) {reverse_iterator __tmp(*this); --current; return __tmp;}
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
     reverse_iterator& operator--() {++current; return *this;}
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
-    reverse_iterator  operator--(int) {reverse_iterator __tmp(*this); ++current; return __tmp;}
+    reverse_iterator operator--(int) {reverse_iterator __tmp(*this); ++current; return __tmp;}
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
-    reverse_iterator  operator+ (difference_type __n) const {return reverse_iterator(current - __n);}
+    reverse_iterator operator+(difference_type __n) const {return reverse_iterator(current - __n);}
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
     reverse_iterator& operator+=(difference_type __n) {current -= __n; return *this;}
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
-    reverse_iterator  operator- (difference_type __n) const {return reverse_iterator(current + __n);}
+    reverse_iterator operator-(difference_type __n) const {return reverse_iterator(current + __n);}
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
     reverse_iterator& operator-=(difference_type __n) {current += __n; return *this;}
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
-    reference         operator[](difference_type __n) const {return *(*this + __n);}
+    reference operator[](difference_type __n) const {return *(*this + __n);}
+
+#if _LIBCPP_STD_VER > 17
+    _LIBCPP_HIDE_FROM_ABI friend constexpr
+    iter_rvalue_reference_t<_Iter> iter_move(const reverse_iterator& __i)
+      noexcept(is_nothrow_copy_constructible_v<_Iter> &&
+          noexcept(ranges::iter_move(--declval<_Iter&>()))) {
+      auto __tmp = __i.base();
+      return ranges::iter_move(--__tmp);
+    }
+
+    template <indirectly_swappable<_Iter> _Iter2>
+    _LIBCPP_HIDE_FROM_ABI friend constexpr
+    void iter_swap(const reverse_iterator& __x, const reverse_iterator<_Iter2>& __y)
+      noexcept(is_nothrow_copy_constructible_v<_Iter> &&
+          is_nothrow_copy_constructible_v<_Iter2> &&
+          noexcept(ranges::iter_swap(--declval<_Iter&>(), --declval<_Iter2&>()))) {
+      auto __xtmp = __x.base();
+      auto __ytmp = __y.base();
+      ranges::iter_swap(--__xtmp, --__ytmp);
+    }
+#endif // _LIBCPP_STD_VER > 17
 };
 
 template <class _Iter1, class _Iter2>
 inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
 bool
 operator==(const reverse_iterator<_Iter1>& __x, const reverse_iterator<_Iter2>& __y)
+#if _LIBCPP_STD_VER > 17
+    requires requires {
+      { __x.base() == __y.base() } -> convertible_to<bool>;
+    }
+#endif // _LIBCPP_STD_VER > 17
 {
     return __x.base() == __y.base();
 }
 
 template <class _Iter1, class _Iter2>
 inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
 bool
 operator<(const reverse_iterator<_Iter1>& __x, const reverse_iterator<_Iter2>& __y)
+#if _LIBCPP_STD_VER > 17
+    requires requires {
+        { __x.base() > __y.base() } -> convertible_to<bool>;
+      }
+#endif // _LIBCPP_STD_VER > 17
 {
     return __x.base() > __y.base();
 }
 
 template <class _Iter1, class _Iter2>
 inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
 bool
 operator!=(const reverse_iterator<_Iter1>& __x, const reverse_iterator<_Iter2>& __y)
+#if _LIBCPP_STD_VER > 17
+    requires requires {
+      { __x.base() != __y.base() } -> convertible_to<bool>;
+    }
+#endif // _LIBCPP_STD_VER > 17
 {
     return __x.base() != __y.base();
 }
 
 template <class _Iter1, class _Iter2>
 inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
 bool
 operator>(const reverse_iterator<_Iter1>& __x, const reverse_iterator<_Iter2>& __y)
+#if _LIBCPP_STD_VER > 17
+    requires requires {
+        { __x.base() < __y.base() } -> convertible_to<bool>;
+      }
+#endif // _LIBCPP_STD_VER > 17
 {
     return __x.base() < __y.base();
 }
 
 template <class _Iter1, class _Iter2>
 inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
 bool
 operator>=(const reverse_iterator<_Iter1>& __x, const reverse_iterator<_Iter2>& __y)
+#if _LIBCPP_STD_VER > 17
+    requires requires {
+        { __x.base() <= __y.base() } -> convertible_to<bool>;
+      }
+#endif // _LIBCPP_STD_VER > 17
 {
     return __x.base() <= __y.base();
 }
 
 template <class _Iter1, class _Iter2>
 inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
 bool
 operator<=(const reverse_iterator<_Iter1>& __x, const reverse_iterator<_Iter2>& __y)
+#if _LIBCPP_STD_VER > 17
+    requires requires {
+        { __x.base() >= __y.base() } -> convertible_to<bool>;
+      }
+#endif // _LIBCPP_STD_VER > 17
 {
     return __x.base() >= __y.base();
 }
 
 #if _LIBCPP_STD_VER > 17
 template <class _Iter1, three_way_comparable_with<_Iter1> _Iter2>
 _LIBCPP_HIDE_FROM_ABI constexpr
 compare_three_way_result_t<_Iter1, _Iter2>

 template <class _Iter>
 inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
 reverse_iterator<_Iter>
 operator+(typename reverse_iterator<_Iter>::difference_type __n, const reverse_iterator<_Iter>& __x)
 {
     return reverse_iterator<_Iter>(__x.base() - __n);
 }
 
+#if _LIBCPP_STD_VER > 17
+template <class _Iter1, class _Iter2>
+  requires (!sized_sentinel_for<_Iter1, _Iter2>)
+inline constexpr bool disable_sized_sentinel_for<reverse_iterator<_Iter1>, reverse_iterator<_Iter2>> = true;
+#endif // _LIBCPP_STD_VER > 17
+
 #if _LIBCPP_STD_VER > 11
 template <class _Iter>
 inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
 reverse_iterator<_Iter> make_reverse_iterator(_Iter __i)
 {
     return reverse_iterator<_Iter>(__i);
 }
 #endif
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___ITERATOR_REVERSE_ITERATOR_H

libcxx/include/iterator

: public iterator<typename iterator_traits<Iterator>::iterator_category, // until C++17
                       typename iterator_traits<Iterator>::value_type,
                       typename iterator_traits<Iterator>::difference_type,
                       typename iterator_traits<Iterator>::pointer,
                       typename iterator_traits<Iterator>::reference>
 {
 protected:
     Iterator current;
 public:
-    typedef Iterator                                            iterator_type;
-    typedef typename iterator_traits<Iterator>::difference_type difference_type;
-    typedef typename iterator_traits<Iterator>::reference       reference;
-    typedef typename iterator_traits<Iterator>::pointer         pointer;
+    using iterator_type     = Iterator;
+    using iterator_concept  = see below; // since C++20
+    using iterator_category = typename iterator_traits<Iterator>::iterator_category; // since C++17, until C++20
+    using iterator_category = see below; // since C++20
+    using value_type        = typename iterator_traits<Iterator>::value_type; // since C++17, until C++20
+    using value_type        = iter_value_t<Iterator>; // since C++20
+    using difference_type   = typename iterator_traits<Iterator>::difference_type; // until C++20
+    using difference_type   = iter_difference_t<Iterator>; // since C++20
+    using pointer           = typename iterator_traits<Iterator>::pointer;
+    using reference         = typename iterator_traits<Iterator>::reference; // until C++20
+    using reference         = iter_reference_t<Iterator>; // since C++20
 
     constexpr reverse_iterator();
     constexpr explicit reverse_iterator(Iterator x);
     template <class U> constexpr reverse_iterator(const reverse_iterator<U>& u);
     template <class U> constexpr reverse_iterator& operator=(const reverse_iterator<U>& u);
     constexpr Iterator base() const;
     constexpr reference operator*() const;
-    constexpr pointer   operator->() const;
+    constexpr pointer   operator->() const; // until C++20
+    constexpr pointer   operator->() const requires see below; // since C++20
     constexpr reverse_iterator& operator++();
     constexpr reverse_iterator  operator++(int);
     constexpr reverse_iterator& operator--();
     constexpr reverse_iterator  operator--(int);
     constexpr reverse_iterator  operator+ (difference_type n) const;
     constexpr reverse_iterator& operator+=(difference_type n);
     constexpr reverse_iterator  operator- (difference_type n) const;
     constexpr reverse_iterator& operator-=(difference_type n);
-    constexpr reference         operator[](difference_type n) const;
+    constexpr unspecified       operator[](difference_type n) const;
+
+    friend constexpr iter_rvalue_reference_t<Iterator>
+      iter_move(const reverse_iterator& i) noexcept(see below);
+    template<indirectly_swappable<Iterator> Iterator2>
+      friend constexpr void
+        iter_swap(const reverse_iterator& x,
+                  const reverse_iterator<Iterator2>& y) noexcept(see below);
 };
 
 template <class Iterator1, class Iterator2>
 constexpr bool                          // constexpr in C++17
 operator==(const reverse_iterator<Iterator1>& x, const reverse_iterator<Iterator2>& y);
 
 template <class Iterator1, class Iterator2>
 constexpr bool                          // constexpr in C++17
-operator<(const reverse_iterator<Iterator1>& x, const reverse_iterator<Iterator2>& y);
+operator!=(const reverse_iterator<Iterator1>& x, const reverse_iterator<Iterator2>& y);
 
 template <class Iterator1, class Iterator2>
 constexpr bool                          // constexpr in C++17
-operator!=(const reverse_iterator<Iterator1>& x, const reverse_iterator<Iterator2>& y);
+operator<(const reverse_iterator<Iterator1>& x, const reverse_iterator<Iterator2>& y);
 
 template <class Iterator1, class Iterator2>
 constexpr bool                          // constexpr in C++17
 operator>(const reverse_iterator<Iterator1>& x, const reverse_iterator<Iterator2>& y);
 
 template <class Iterator1, class Iterator2>
 constexpr bool                          // constexpr in C++17
-operator>=(const reverse_iterator<Iterator1>& x, const reverse_iterator<Iterator2>& y);
+operator<=(const reverse_iterator<Iterator1>& x, const reverse_iterator<Iterator2>& y);
 
 template <class Iterator1, class Iterator2>
 constexpr bool                          // constexpr in C++17
-operator<=(const reverse_iterator<Iterator1>& x, const reverse_iterator<Iterator2>& y);
+operator>=(const reverse_iterator<Iterator1>& x, const reverse_iterator<Iterator2>& y);
+
+template<class Iterator1, three_way_comparable_with<Iterator1> Iterator2>
+  constexpr compare_three_way_result_t<Iterator1, Iterator2>
+    operator<=>(const reverse_iterator<Iterator1>& x,
+                const reverse_iterator<Iterator2>& y);
 
 template <class Iterator1, class Iterator2>
 constexpr auto
 operator-(const reverse_iterator<Iterator1>& x, const reverse_iterator<Iterator2>& y)
 -> decltype(__y.base() - __x.base());   // constexpr in C++17
 
 template <class Iterator>
 constexpr reverse_iterator<Iterator>
 operator+(typename reverse_iterator<Iterator>::difference_type n,
           const reverse_iterator<Iterator>& x);   // constexpr in C++17
 
 template <class Iterator>
 constexpr reverse_iterator<Iterator> make_reverse_iterator(Iterator i); // C++14, constexpr in C++17
 
+template<class Iterator1, class Iterator2>
+    requires (!sized_sentinel_for<Iterator1, Iterator2>)
+  inline constexpr bool disable_sized_sentinel_for<reverse_iterator<Iterator1>,
+                                                   reverse_iterator<Iterator2>> = true;
+
 template <class Container>
 class back_insert_iterator
     : public iterator<output_iterator_tag, void, void, void, void> // until C++17
 {
 protected:
     Container* container;
 public:
     typedef Container                   container_type;

libcxx/test/libcxx/iterators/predef.iterators/reverse.iterators/bad_template_argument.verify.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// UNSUPPORTED: libcpp-no-concepts
+
+// <iterator>
+
+// reverse_iterator
+
+#include <iterator>
+
+#include "test_iterators.h"
+
+int main(int, char**) {
+  using BadIter = std::reverse_iterator<forward_iterator<int*>>;
+  BadIter i; //expected-error-re@*:* {{static_assert failed{{.*}} "reverse_iterator<It> requires It to be a bidirectional iterator."}}
+
+  return 0;
+}

libcxx/test/std/iterators/predef.iterators/reverse.iterators/iterator_concept_conformance.compile.pass.cpp

 #include <iterator>
 
 #include "test_iterators.h"
 
 template<class I1>
 constexpr bool common_reverse_iterator_checks() {
   static_assert(std::indirectly_writable<I1, int>);
   static_assert(std::sentinel_for<I1, I1>);
-  static_assert(std::sentinel_for<I1, std::reverse_iterator<float*>>);
-  static_assert(!std::sized_sentinel_for<I1, std::reverse_iterator<float*>>);
   return true;
 }
 
 using reverse_bidirectional_iterator = std::reverse_iterator<bidirectional_iterator<int*>>;
 static_assert(common_reverse_iterator_checks<reverse_bidirectional_iterator>());
 static_assert(std::bidirectional_iterator<reverse_bidirectional_iterator>);
 static_assert(!std::random_access_iterator<reverse_bidirectional_iterator>);
 static_assert(!std::sized_sentinel_for<reverse_bidirectional_iterator, reverse_bidirectional_iterator>);

 static_assert(std::random_access_iterator<reverse_contiguous_iterator>);
 static_assert(!std::contiguous_iterator<reverse_contiguous_iterator>);
 static_assert(std::sized_sentinel_for<reverse_contiguous_iterator, reverse_contiguous_iterator>);
 static_assert( std::indirectly_movable<reverse_contiguous_iterator, reverse_contiguous_iterator>);
 static_assert( std::indirectly_movable_storable<reverse_contiguous_iterator, reverse_contiguous_iterator>);
 static_assert( std::indirectly_copyable<reverse_contiguous_iterator, reverse_contiguous_iterator>);
 static_assert( std::indirectly_copyable_storable<reverse_contiguous_iterator, reverse_contiguous_iterator>);
 static_assert( std::indirectly_swappable<reverse_contiguous_iterator, reverse_contiguous_iterator>);
+
+static_assert( std::equality_comparable_with<std::reverse_iterator<int*>, std::reverse_iterator<const int*>>);
+static_assert(!std::equality_comparable_with<std::reverse_iterator<int*>, std::reverse_iterator<char*>>);
+static_assert( std::totally_ordered_with<std::reverse_iterator<int*>, std::reverse_iterator<const int*>>);
+static_assert(!std::totally_ordered_with<std::reverse_iterator<int*>, std::reverse_iterator<char*>>);
+static_assert( std::three_way_comparable_with<std::reverse_iterator<int*>, std::reverse_iterator<const int*>>);
+static_assert(!std::three_way_comparable_with<std::reverse_iterator<int*>, std::reverse_iterator<char*>>);

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/sfinae.compile.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// UNSUPPORTED: libcpp-no-concepts
+
+// <iterator>
+//
+// reverse_iterator
+//
+// template <class Iterator1, class Iterator2>
+// constexpr bool                          // constexpr in C++17
+// operator==(const reverse_iterator<Iterator1>& x, const reverse_iterator<Iterator2>& y);
+//
+// template <class Iterator1, class Iterator2>
+// constexpr bool                          // constexpr in C++17
+// operator!=(const reverse_iterator<Iterator1>& x, const reverse_iterator<Iterator2>& y);
+//
+// template <class Iterator1, class Iterator2>
+// constexpr bool                          // constexpr in C++17
+// operator<(const reverse_iterator<Iterator1>& x, const reverse_iterator<Iterator2>& y);
+//
+// template <class Iterator1, class Iterator2>
+// constexpr bool                          // constexpr in C++17
+// operator>(const reverse_iterator<Iterator1>& x, const reverse_iterator<Iterator2>& y);
+//
+// template <class Iterator1, class Iterator2>
+// constexpr bool                          // constexpr in C++17
+// operator<=(const reverse_iterator<Iterator1>& x, const reverse_iterator<Iterator2>& y);
+//
+// template <class Iterator1, class Iterator2>
+// constexpr bool                          // constexpr in C++17
+// operator>=(const reverse_iterator<Iterator1>& x, const reverse_iterator<Iterator2>& y);
+//
+// template<class Iterator1, three_way_comparable_with<Iterator1> Iterator2>
+//  constexpr compare_three_way_result_t<Iterator1, Iterator2>
+//    operator<=>(const reverse_iterator<Iterator1>& x,
+//                const reverse_iterator<Iterator2>& y);
+
+#include <iterator>
+#include <cassert>
+
+#include "test_macros.h"
+
+struct IterBase {
+  using iterator_category = std::bidirectional_iterator_tag;
+  using value_type = int;
+  using difference_type = ptrdiff_t;
+  using pointer = int*;
+  using reference = int&;
+
+  reference operator*() const;
+  pointer operator->() const;
+};
+
+template<class T> concept HasEqual = requires (T t) { t == t; };
+template<class T> concept HasNotEqual = requires (T t) { t != t; };
+template<class T> concept HasLess = requires (T t) { t < t; };
+template<class T> concept HasLessOrEqual = requires (T t) { t <= t; };
+template<class T> concept HasGreater = requires (T t) { t > t; };
+template<class T> concept HasGreaterOrEqual = requires (T t) { t >= t; };
+template<class T> concept HasSpaceship = requires (T t) { t <=> t; };
+
+// operator ==
+
+struct NoEqualityCompIter : IterBase {
+  bool operator!=(NoEqualityCompIter) const;
+  bool operator<(NoEqualityCompIter) const;
+  bool operator>(NoEqualityCompIter) const;
+  bool operator<=(NoEqualityCompIter) const;
+  bool operator>=(NoEqualityCompIter) const;
+};
+
+static_assert( HasEqual<std::reverse_iterator<int*>>);
+static_assert(!HasEqual<std::reverse_iterator<NoEqualityCompIter>>);
+static_assert( HasNotEqual<std::reverse_iterator<NoEqualityCompIter>>);
+static_assert( HasLess<std::reverse_iterator<NoEqualityCompIter>>);
+static_assert( HasLessOrEqual<std::reverse_iterator<NoEqualityCompIter>>);
+static_assert( HasGreater<std::reverse_iterator<NoEqualityCompIter>>);
+static_assert( HasGreaterOrEqual<std::reverse_iterator<NoEqualityCompIter>>);
+
+void Foo() {
+  std::reverse_iterator<NoEqualityCompIter> i;
+  (void)i;
+}
+
+// operator !=
+
+struct NoInequalityCompIter : IterBase {
+  bool operator<(NoInequalityCompIter) const;
+  bool operator>(NoInequalityCompIter) const;
+  bool operator<=(NoInequalityCompIter) const;
+  bool operator>=(NoInequalityCompIter) const;
+};
+
+static_assert( HasNotEqual<std::reverse_iterator<int*>>);
+static_assert(!HasNotEqual<std::reverse_iterator<NoInequalityCompIter>>);
+static_assert(!HasEqual<std::reverse_iterator<NoInequalityCompIter>>);
+static_assert( HasLess<std::reverse_iterator<NoInequalityCompIter>>);
+static_assert( HasLessOrEqual<std::reverse_iterator<NoInequalityCompIter>>);
+static_assert( HasGreater<std::reverse_iterator<NoInequalityCompIter>>);
+static_assert( HasGreaterOrEqual<std::reverse_iterator<NoInequalityCompIter>>);
+
+// operator <
+
+struct NoGreaterCompIter : IterBase {
+  bool operator==(NoGreaterCompIter) const;
+  bool operator!=(NoGreaterCompIter) const;
+  bool operator<(NoGreaterCompIter) const;
+  bool operator<=(NoGreaterCompIter) const;
+  bool operator>=(NoGreaterCompIter) const;
+};
+
+static_assert( HasLess<std::reverse_iterator<int*>>);
+static_assert(!HasLess<std::reverse_iterator<NoGreaterCompIter>>);
+static_assert( HasEqual<std::reverse_iterator<NoGreaterCompIter>>);
+static_assert( HasNotEqual<std::reverse_iterator<NoGreaterCompIter>>);
+static_assert( HasLessOrEqual<std::reverse_iterator<NoGreaterCompIter>>);
+static_assert( HasGreater<std::reverse_iterator<NoGreaterCompIter>>);
+static_assert( HasGreaterOrEqual<std::reverse_iterator<NoGreaterCompIter>>);
+
+// operator >
+
+struct NoLessCompIter : IterBase {
+  bool operator==(NoLessCompIter) const;
+  bool operator!=(NoLessCompIter) const;
+  bool operator>(NoLessCompIter) const;
+  bool operator<=(NoLessCompIter) const;
+  bool operator>=(NoLessCompIter) const;
+};
+
+static_assert( HasGreater<std::reverse_iterator<int*>>);
+static_assert(!HasGreater<std::reverse_iterator<NoLessCompIter>>);
+static_assert( HasEqual<std::reverse_iterator<NoLessCompIter>>);
+static_assert( HasNotEqual<std::reverse_iterator<NoLessCompIter>>);
+static_assert( HasLess<std::reverse_iterator<NoLessCompIter>>);
+static_assert( HasLessOrEqual<std::reverse_iterator<NoLessCompIter>>);
+static_assert( HasGreaterOrEqual<std::reverse_iterator<NoLessCompIter>>);
+
+// operator <=
+
+struct NoGreaterOrEqualCompIter : IterBase {
+  bool operator==(NoGreaterOrEqualCompIter) const;
+  bool operator!=(NoGreaterOrEqualCompIter) const;
+  bool operator<(NoGreaterOrEqualCompIter) const;
+  bool operator>(NoGreaterOrEqualCompIter) const;
+  bool operator<=(NoGreaterOrEqualCompIter) const;
+};
+
+static_assert( HasLessOrEqual<std::reverse_iterator<int*>>);
+static_assert(!HasLessOrEqual<std::reverse_iterator<NoGreaterOrEqualCompIter>>);
+static_assert( HasEqual<std::reverse_iterator<NoGreaterOrEqualCompIter>>);
+static_assert( HasNotEqual<std::reverse_iterator<NoGreaterOrEqualCompIter>>);
+static_assert( HasLess<std::reverse_iterator<NoGreaterOrEqualCompIter>>);
+static_assert( HasGreater<std::reverse_iterator<NoGreaterOrEqualCompIter>>);
+static_assert( HasGreaterOrEqual<std::reverse_iterator<NoGreaterOrEqualCompIter>>);
+
+// operator >=
+
+struct NoLessOrEqualCompIter : IterBase {
+  bool operator==(NoLessOrEqualCompIter) const;
+  bool operator!=(NoLessOrEqualCompIter) const;
+  bool operator<(NoLessOrEqualCompIter) const;
+  bool operator>(NoLessOrEqualCompIter) const;
+  bool operator>=(NoLessOrEqualCompIter) const;
+};
+
+static_assert( HasGreaterOrEqual<std::reverse_iterator<int*>>);
+static_assert(!HasGreaterOrEqual<std::reverse_iterator<NoLessOrEqualCompIter>>);
+static_assert( HasEqual<std::reverse_iterator<NoLessOrEqualCompIter>>);
+static_assert( HasNotEqual<std::reverse_iterator<NoLessOrEqualCompIter>>);
+static_assert( HasLess<std::reverse_iterator<NoLessOrEqualCompIter>>);
+static_assert( HasLessOrEqual<std::reverse_iterator<NoLessOrEqualCompIter>>);
+static_assert( HasGreater<std::reverse_iterator<NoLessOrEqualCompIter>>);
+
+// operator <=>
+
+static_assert( std::three_way_comparable_with<int*, int*>);
+static_assert( HasSpaceship<std::reverse_iterator<int*>>);
+static_assert(!std::three_way_comparable_with<NoEqualityCompIter, NoEqualityCompIter>);
+static_assert(!HasSpaceship<std::reverse_iterator<NoEqualityCompIter>>);
+static_assert(!std::three_way_comparable_with<NoInequalityCompIter, NoInequalityCompIter>);
+static_assert(!HasSpaceship<std::reverse_iterator<NoInequalityCompIter>>);
+static_assert(!std::three_way_comparable_with<NoGreaterCompIter, NoGreaterCompIter>);
+static_assert(!HasSpaceship<std::reverse_iterator<NoGreaterCompIter>>);
+static_assert(!std::three_way_comparable_with<NoLessCompIter, NoLessCompIter>);
+static_assert(!HasSpaceship<std::reverse_iterator<NoLessCompIter>>);
+static_assert(!std::three_way_comparable_with<NoGreaterOrEqualCompIter, NoGreaterOrEqualCompIter>);
+static_assert(!HasSpaceship<std::reverse_iterator<NoGreaterOrEqualCompIter>>);
+static_assert(!std::three_way_comparable_with<NoLessOrEqualCompIter, NoLessOrEqualCompIter>);
+static_assert(!HasSpaceship<std::reverse_iterator<NoLessOrEqualCompIter>>);

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.elem/arrow.pass.cpp

public:
     friend TEST_CONSTEXPR bool operator==(const C& x, const C& y)
         {return x.data_ == y.data_;}
 };
 
 TEST_CONSTEXPR  C gC;
 
 int main(int, char**)
 {
-    A a;
-    test(&a+1, A());
 
-    {
     std::list<B> l;
     l.push_back(B(0));
     l.push_back(B(1));
     l.push_back(B(2));
 
     {
       std::list<B>::const_iterator i = l.begin();
       assert ( i->get() == 0 );  ++i;
       assert ( i->get() == 1 );  ++i;
       assert ( i->get() == 2 );  ++i;
       assert ( i == l.end ());
     }
 
     {
       std::list<B>::const_reverse_iterator ri = l.rbegin();
       assert ( ri->get() == 2 );  ++ri;
       assert ( ri->get() == 1 );  ++ri;
       assert ( ri->get() == 0 );  ++ri;
       assert ( ri == l.rend ());
     }
-    }
 
 #if TEST_STD_VER > 14
-    {
-        typedef std::reverse_iterator<const C *> RI;
-        constexpr RI it1 = std::make_reverse_iterator(&gC+1);
 
-        static_assert(it1->get() == gC.get(), "");
-    }
 #endif
-    {
-        ((void)gC);
-    }
 
   return 0;
 }

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.elem/arrow.sfinae.compile.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
+// <iterator>
+//
+// reverse_iterator
+//
+// pointer operator->() const;
+
+#include <iterator>
+
+#include <type_traits>
+#include "test_iterators.h"
+
+template <class T>
+concept HasArrow = requires(T t) { t.operator->(); };
+
+struct simple_bidirectional_iterator {
+    using iterator_category = std::bidirectional_iterator_tag;
+    using value_type = int;
+    using difference_type = int;
+    using pointer = int*;
+    using reference = int&;
+
+    reference operator*() const;
+    pointer operator->() const;
+
+    simple_bidirectional_iterator& operator++();
+    simple_bidirectional_iterator& operator--();
+    simple_bidirectional_iterator operator++(int);
+    simple_bidirectional_iterator operator--(int);
+
+    friend bool operator==(const simple_bidirectional_iterator&, const simple_bidirectional_iterator&);
+};
+static_assert( std::bidirectional_iterator<simple_bidirectional_iterator>);
+static_assert(!std::random_access_iterator<simple_bidirectional_iterator>);
+
+using PtrRI = std::reverse_iterator<int*>;
+static_assert( HasArrow<PtrRI>);
+
+using PtrLikeRI = std::reverse_iterator<simple_bidirectional_iterator>;
+static_assert( HasArrow<PtrLikeRI>);
+
+// `bidirectional_iterator` from `test_iterators.h` doesn't define `operator->`.
+using NonPtrRI = std::reverse_iterator<bidirectional_iterator<int*>>;
+static_assert(!HasArrow<NonPtrRI>);

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/iter_move.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
+// <iterator>
+//
+// reverse_iterator
+//
+// friend constexpr iter_rvalue_reference_t<Iterator>
+//   iter_move(const reverse_iterator& i) noexcept(see below);
+
+#include <iterator>
+
+#include <cassert>
+#include <type_traits>
+#include <utility>
+#include "test_macros.h"
+
+namespace adl {
+
+struct Iterator {
+  using value_type = int;
+  using difference_type = ptrdiff_t;
+
+  value_type* ptr_ = nullptr;
+  int* iter_move_invocations_ = nullptr;
+
+  constexpr Iterator() = default;
+  constexpr explicit Iterator(int* p, int& iter_moves) : ptr_(p), iter_move_invocations_(&iter_moves) {}
+
+  constexpr value_type& operator*() const { return *ptr_; }
+
+  Iterator& operator++() { ++ptr_; return *this; }
+  Iterator operator++(int) {
+    Iterator prev = *this;
+    ++ptr_;
+    return prev;
+  }
+
+  constexpr Iterator& operator--() { --ptr_; return *this; }
+  constexpr Iterator operator--(int) {
+    Iterator prev = *this;
+    --ptr_;
+    return prev;
+  }
+
+  constexpr friend value_type&& iter_move(Iterator iter) {
+    if (iter.iter_move_invocations_) {
+      ++(*iter.iter_move_invocations_);
+    }
+    return std::move(*iter);
+  }
+
+  friend bool operator==(const Iterator& lhs, const Iterator& rhs) { return lhs.ptr_ == rhs.ptr_; }
+};
+
+} // namespace adl
+
+constexpr bool test() {
+  // Can use `iter_move` with a regular array.
+  {
+    constexpr int N = 3;
+    int a[N] = {0, 1, 2};
+
+    std::reverse_iterator<int*> ri(a + N);
+    static_assert(std::same_as<decltype(iter_move(ri)), int&&>);
+    assert(iter_move(ri) == 2);
+
+    ++ri;
+    assert(iter_move(ri) == 1);
+  }
+
+  // Ensure the `iter_move` customization point is being used.
+  {
+    constexpr int N = 3;
+    int a[N] = {0, 1, 2};
+
+    int iter_move_invocations = 0;
+    adl::Iterator i(a + N, iter_move_invocations);
+    std::reverse_iterator<adl::Iterator> ri(i);
+    int x = iter_move(ri);
+    assert(x == 2);
+    assert(iter_move_invocations == 1);
+  }
+
+  // Check the `noexcept` specification.
+  {
+    {
+      struct ThrowingCopyNoexceptDecrement {
+        using value_type = int;
+        using difference_type = ptrdiff_t;
+
+        ThrowingCopyNoexceptDecrement();
+        ThrowingCopyNoexceptDecrement(const ThrowingCopyNoexceptDecrement&);
+
+        int& operator*() const noexcept { static int x; return x; }
+
+        ThrowingCopyNoexceptDecrement& operator++();
+        ThrowingCopyNoexceptDecrement operator++(int);
+        ThrowingCopyNoexceptDecrement& operator--() noexcept;
+        ThrowingCopyNoexceptDecrement operator--(int) noexcept;
+
+        bool operator==(const ThrowingCopyNoexceptDecrement&) const = default;
+      };
+      static_assert(std::bidirectional_iterator<ThrowingCopyNoexceptDecrement>);
+
+      static_assert(!std::is_nothrow_copy_constructible_v<ThrowingCopyNoexceptDecrement>);
+      ASSERT_NOEXCEPT(std::ranges::iter_move(--std::declval<ThrowingCopyNoexceptDecrement&>()));
+      using RI = std::reverse_iterator<ThrowingCopyNoexceptDecrement>;
+      ASSERT_NOT_NOEXCEPT(iter_move(std::declval<RI>()));
+    }
+
+    {
+      struct NoexceptCopyThrowingDecrement {
+        using value_type = int;
+        using difference_type = ptrdiff_t;
+
+        NoexceptCopyThrowingDecrement();
+        NoexceptCopyThrowingDecrement(const NoexceptCopyThrowingDecrement&) noexcept;
+
+        int& operator*() const { static int x; return x; }
+
+        NoexceptCopyThrowingDecrement& operator++();
+        NoexceptCopyThrowingDecrement operator++(int);
+        NoexceptCopyThrowingDecrement& operator--();
+        NoexceptCopyThrowingDecrement operator--(int);
+
+        bool operator==(const NoexceptCopyThrowingDecrement&) const = default;
+      };
+      static_assert(std::bidirectional_iterator<NoexceptCopyThrowingDecrement>);
+
+      static_assert( std::is_nothrow_copy_constructible_v<NoexceptCopyThrowingDecrement>);
+      ASSERT_NOT_NOEXCEPT(std::ranges::iter_move(--std::declval<NoexceptCopyThrowingDecrement&>()));
+      using RI = std::reverse_iterator<NoexceptCopyThrowingDecrement>;
+      ASSERT_NOT_NOEXCEPT(iter_move(std::declval<RI>()));
+    }
+
+    {
+      struct NoexceptCopyAndDecrement {
+        using value_type = int;
+        using difference_type = ptrdiff_t;
+
+        NoexceptCopyAndDecrement();
+        NoexceptCopyAndDecrement(const NoexceptCopyAndDecrement&) noexcept;
+
+        int& operator*() const noexcept { static int x; return x; }
+
+        NoexceptCopyAndDecrement& operator++();
+        NoexceptCopyAndDecrement operator++(int);
+        NoexceptCopyAndDecrement& operator--() noexcept;
+        NoexceptCopyAndDecrement operator--(int) noexcept;
+
+        bool operator==(const NoexceptCopyAndDecrement&) const = default;
+      };
+      static_assert(std::bidirectional_iterator<NoexceptCopyAndDecrement>);
+
+      static_assert( std::is_nothrow_copy_constructible_v<NoexceptCopyAndDecrement>);
+      ASSERT_NOEXCEPT(std::ranges::iter_move(--std::declval<NoexceptCopyAndDecrement&>()));
+      using RI = std::reverse_iterator<NoexceptCopyAndDecrement>;
+      ASSERT_NOEXCEPT(iter_move(std::declval<RI>()));
+    }
+  }
+
+  return true;
+}
+
+int main(int, char**) {
+  test();
+  static_assert(test());
+
+  return 0;
+}

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.nonmember/iter_swap.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
+// <iterator>
+//
+// reverse_iterator
+//
+// template<indirectly_swappable<Iterator> Iterator2>
+//   friend constexpr void
+//     iter_swap(const reverse_iterator& x,
+//               const reverse_iterator<Iterator2>& y) noexcept(see below);
+
+#include <iterator>
+
+#include <cassert>
+#include <type_traits>
+#include <utility>
+#include "test_macros.h"
+
+namespace adl {
+
+struct Iterator {
+  using value_type = int;
+  using difference_type = ptrdiff_t;
+
+  value_type* ptr_ = nullptr;
+  int* iter_swap_invocations_ = nullptr;
+
+  constexpr Iterator() = default;
+  constexpr explicit Iterator(int& iter_swaps) : iter_swap_invocations_(&iter_swaps) {}
+
+  value_type& operator*() const { return *ptr_; }
+
+  Iterator& operator++() { ++ptr_; return *this; }
+  Iterator operator++(int) {
+    Iterator prev = *this;
+    ++ptr_;
+    return prev;
+  }
+
+  Iterator& operator--() { --ptr_; return *this; }
+  Iterator operator--(int) {
+    Iterator prev = *this;
+    --ptr_;
+    return prev;
+  }
+
+  constexpr friend void iter_swap(Iterator a, Iterator) {
+    if (a.iter_swap_invocations_) {
+      ++(*a.iter_swap_invocations_);
+    }
+  }
+
+  friend bool operator==(const Iterator& lhs, const Iterator& rhs) { return lhs.ptr_ == rhs.ptr_; }
+};
+
+} // namespace adl
+
+constexpr bool test() {
+  // Can use `iter_swap` with a regular array.
+  {
+    constexpr int N = 3;
+    int a[N] = {0, 1, 2};
+
+    std::reverse_iterator rb(a + N);
+    std::reverse_iterator re(a + 1);
+    assert(a[0] == 0);
+    assert(a[2] == 2);
+
+    static_assert(std::same_as<decltype(iter_swap(rb, re)), void>);
+    iter_swap(rb, re);
+    assert(a[0] == 2);
+    assert(a[2] == 0);
+  }
+
+  // Ensure the `iter_swap` customization point is being used.
+  {
+    int iter_swap_invocations = 0;
+    adl::Iterator i1(iter_swap_invocations), i2(iter_swap_invocations);
+    std::reverse_iterator<adl::Iterator> ri1(i1), ri2(i2);
+    iter_swap(i1, i2);
+    assert(iter_swap_invocations == 1);
+
+    iter_swap(i2, i1);
+    assert(iter_swap_invocations == 2);
+  }
+
+  // Check the `noexcept` specification.
+  {
+    {
+      struct ThrowingCopyNoexceptDecrement {
+        using value_type = int;
+        using difference_type = ptrdiff_t;
+
+        ThrowingCopyNoexceptDecrement();
+        ThrowingCopyNoexceptDecrement(const ThrowingCopyNoexceptDecrement&);
+
+        int& operator*() const noexcept { static int x; return x; }
+
+        ThrowingCopyNoexceptDecrement& operator++();
+        ThrowingCopyNoexceptDecrement operator++(int);
+        ThrowingCopyNoexceptDecrement& operator--() noexcept;
+        ThrowingCopyNoexceptDecrement operator--(int) noexcept;
+
+        bool operator==(const ThrowingCopyNoexceptDecrement&) const = default;
+      };
+      static_assert(std::bidirectional_iterator<ThrowingCopyNoexceptDecrement>);
+
+      static_assert(!std::is_nothrow_copy_constructible_v<ThrowingCopyNoexceptDecrement>);
+      static_assert( std::is_nothrow_copy_constructible_v<int*>);
+      ASSERT_NOEXCEPT(std::ranges::iter_swap(--std::declval<ThrowingCopyNoexceptDecrement&>(), --std::declval<int*&>()));
+      using RI1 = std::reverse_iterator<ThrowingCopyNoexceptDecrement>;
+      using RI2 = std::reverse_iterator<int*>;
+      ASSERT_NOT_NOEXCEPT(iter_swap(std::declval<RI1>(), std::declval<RI2>()));
+      ASSERT_NOT_NOEXCEPT(iter_swap(std::declval<RI2>(), std::declval<RI1>()));
+    }
+
+    {
+      struct NoexceptCopyThrowingDecrement {
+        using value_type = int;
+        using difference_type = ptrdiff_t;
+
+        NoexceptCopyThrowingDecrement();
+        NoexceptCopyThrowingDecrement(const NoexceptCopyThrowingDecrement&) noexcept;
+
+        int& operator*() const { static int x; return x; }
+
+        NoexceptCopyThrowingDecrement& operator++();
+        NoexceptCopyThrowingDecrement operator++(int);
+        NoexceptCopyThrowingDecrement& operator--();
+        NoexceptCopyThrowingDecrement operator--(int);
+
+        bool operator==(const NoexceptCopyThrowingDecrement&) const = default;
+      };
+      static_assert(std::bidirectional_iterator<NoexceptCopyThrowingDecrement>);
+
+      static_assert( std::is_nothrow_copy_constructible_v<NoexceptCopyThrowingDecrement>);
+      static_assert( std::is_nothrow_copy_constructible_v<int*>);
+      ASSERT_NOT_NOEXCEPT(std::ranges::iter_swap(--std::declval<NoexceptCopyThrowingDecrement&>(), --std::declval<int*&>()));
+      using RI1 = std::reverse_iterator<NoexceptCopyThrowingDecrement>;
+      using RI2 = std::reverse_iterator<int*>;
+      ASSERT_NOT_NOEXCEPT(iter_swap(std::declval<RI1>(), std::declval<RI2>()));
+      ASSERT_NOT_NOEXCEPT(iter_swap(std::declval<RI2>(), std::declval<RI1>()));
+    }
+
+    {
+      struct NoexceptCopyAndDecrement {
+        using value_type = int;
+        using difference_type = ptrdiff_t;
+
+        NoexceptCopyAndDecrement();
+        NoexceptCopyAndDecrement(const NoexceptCopyAndDecrement&) noexcept;
+
+        int& operator*() const noexcept { static int x; return x; }
+
+        NoexceptCopyAndDecrement& operator++();
+        NoexceptCopyAndDecrement operator++(int);
+        NoexceptCopyAndDecrement& operator--() noexcept;
+        NoexceptCopyAndDecrement operator--(int) noexcept;
+
+        bool operator==(const NoexceptCopyAndDecrement&) const = default;
+      };
+      static_assert(std::bidirectional_iterator<NoexceptCopyAndDecrement>);
+
+      static_assert( std::is_nothrow_copy_constructible_v<NoexceptCopyAndDecrement>);
+      static_assert( std::is_nothrow_copy_constructible_v<int*>);
+      ASSERT_NOEXCEPT(std::ranges::iter_swap(--std::declval<NoexceptCopyAndDecrement&>(), --std::declval<int*&>()));
+      using RI1 = std::reverse_iterator<NoexceptCopyAndDecrement>;
+      using RI2 = std::reverse_iterator<int*>;
+      ASSERT_NOEXCEPT(iter_swap(std::declval<RI1>(), std::declval<RI2>()));
+      ASSERT_NOEXCEPT(iter_swap(std::declval<RI2>(), std::declval<RI1>()));
+    }
+  }
+
+  return true;
+}
+
+int main(int, char**) {
+  test();
+  static_assert(test());
+
+  return 0;
+}

libcxx/test/std/iterators/predef.iterators/reverse.iterators/sized_sentinel.compile.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// UNSUPPORTED: libcpp-no-concepts
+
+// reverse_iterator
+
+#include <iterator>
+
+#include "test_iterators.h"
+
+template<class T> concept HasMinus = requires (T t) { t - t; };
+
+using sized_it = random_access_iterator<int*>;
+static_assert( std::sized_sentinel_for<sized_it, sized_it>);
+static_assert( std::sized_sentinel_for<std::reverse_iterator<sized_it>, std::reverse_iterator<sized_it>>);
+static_assert( HasMinus<std::reverse_iterator<sized_it>>);
+
+// Check that `sized_sentinel_for` is false for `reverse_iterator`s if it is false for the underlying iterators.
+using unsized_it = bidirectional_iterator<int*>;
+static_assert(!std::sized_sentinel_for<unsized_it, unsized_it>);
+static_assert(!std::sized_sentinel_for<std::reverse_iterator<unsized_it>, std::reverse_iterator<unsized_it>>);
+static_assert(!HasMinus<std::reverse_iterator<unsized_it>>);

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

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <iterator>
+
+// reverse_iterator
+
+// Test nested types and data member:
+
+// template <BidirectionalIterator Iter>
+// class reverse_iterator {
+// protected:
+//   Iter current;
+// public:
+//   iterator<typename iterator_traits<Iterator>::iterator_category,
+//   typename iterator_traits<Iterator>::value_type,
+//   typename iterator_traits<Iterator>::difference_type,
+//   typename iterator_traits<Iterator>::pointer,
+//   typename iterator_traits<Iterator>::reference> {
+// };
+
+#include <iterator>
+#include <type_traits>
+
+#include "test_macros.h"
+#include "test_iterators.h"
+
+template <class It>
+struct find_current
+    : private std::reverse_iterator<It>
+{
+    void test() { (void)this->current; }
+};
+
+template <class It>
+void test() {
+  typedef std::reverse_iterator<It> R;
+  typedef std::iterator_traits<It> T;
+  find_current<It> q; q.test(); // Just test that we can access `.current` from derived classes
+  static_assert((std::is_same<typename R::iterator_type, It>::value), "");
+  static_assert((std::is_same<typename R::value_type, typename T::value_type>::value), "");
+  static_assert((std::is_same<typename R::difference_type, typename T::difference_type>::value), "");
+  static_assert((std::is_same<typename R::reference, typename T::reference>::value), "");
+  static_assert((std::is_same<typename R::pointer, typename std::iterator_traits<It>::pointer>::value), "");
+
+#if TEST_STD_VER <= 14
+  typedef std::iterator<typename T::iterator_category, typename T::value_type> iterator_base;
+  static_assert((std::is_base_of<iterator_base, R>::value), "");
+#endif
+#if TEST_STD_VER > 17
+  if constexpr (std::is_same_v<typename T::iterator_category, std::contiguous_iterator_tag>) {
+    static_assert((std::is_same<typename R::iterator_category, std::random_access_iterator_tag>::value), "");
+  } else {
+    static_assert((std::is_same<typename R::iterator_category, typename T::iterator_category>::value), "");
+  }
+#else
+  static_assert((std::is_same<typename R::iterator_category, typename T::iterator_category>::value), "");
+#endif
+}
+
+#if TEST_STD_VER > 17
+
+struct FooIter {
+  using iterator_category = std::bidirectional_iterator_tag;
+  using value_type = void*;
+  using difference_type = void*;
+  using pointer = void*;
+  using reference = int&;
+  int& operator*() const;
+};
+template <>
+struct std::indirectly_readable_traits<FooIter> {
+  using value_type = int;
+};
+template <>
+struct std::incrementable_traits<FooIter> {
+  using difference_type = char;
+};
+
+static_assert(std::is_same_v<typename std::reverse_iterator<FooIter>::value_type, int>);
+static_assert(std::is_same_v<typename std::reverse_iterator<FooIter>::difference_type, char>);
+
+#endif
+
+struct BarIter {
+  bool& operator*() const;
+};
+template <>
+struct std::iterator_traits<BarIter> {
+  using difference_type = char;
+  using value_type = char;
+  using pointer = char*;
+  using reference = char&;
+  using iterator_category = std::bidirectional_iterator_tag;
+};
+
+#if TEST_STD_VER > 17
+  static_assert(std::is_same_v<typename std::reverse_iterator<BarIter>::reference, bool&>);
+#else
+  static_assert(std::is_same<typename std::reverse_iterator<BarIter>::reference, char&>::value, "");
+#endif
+
+void test_all() {
+  test<bidirectional_iterator<char*> >();
+  test<random_access_iterator<char*> >();
+  test<char*>();
+
+#if TEST_STD_VER > 17
+  test<contiguous_iterator<char*>>();
+  static_assert(std::is_same_v<typename std::reverse_iterator<bidirectional_iterator<char*>>::iterator_concept, std::bidirectional_iterator_tag>);
+  static_assert(std::is_same_v<typename std::reverse_iterator<random_access_iterator<char*>>::iterator_concept, std::random_access_iterator_tag>);
+  static_assert(std::is_same_v<typename std::reverse_iterator<contiguous_iterator<char*>>::iterator_concept, std::random_access_iterator_tag>);
+  static_assert(std::is_same_v<typename std::reverse_iterator<char*>::iterator_concept, std::random_access_iterator_tag>);
+#endif
+}

libcxx/test/std/iterators/predef.iterators/reverse.iterators/types.pass.cpp

-//===----------------------------------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-// <iterator>
-
-// reverse_iterator
-
-// Test nested types and data member:
-
-// template <BidirectionalIterator Iter>
-// class reverse_iterator {
-// protected:
-//   Iter current;
-// public:
-//   iterator<typename iterator_traits<Iterator>::iterator_category,
-//   typename iterator_traits<Iterator>::value_type,
-//   typename iterator_traits<Iterator>::difference_type,
-//   typename iterator_traits<Iterator>::pointer,
-//   typename iterator_traits<Iterator>::reference> {
-// };
-
-#include <iterator>
-#include <type_traits>
-
-#include "test_macros.h"
-#include "test_iterators.h"
-
-template <class It>
-struct find_current
-    : private std::reverse_iterator<It>
-{
-    void test() { (void)this->current; }
-};
-
-template <class It>
-void
-test()
-{
-    typedef std::reverse_iterator<It> R;
-    typedef std::iterator_traits<It> T;
-    find_current<It> q; q.test(); // Just test that we can access `.current` from derived classes
-    static_assert((std::is_same<typename R::iterator_type, It>::value), "");
-    static_assert((std::is_same<typename R::value_type, typename T::value_type>::value), "");
-    static_assert((std::is_same<typename R::difference_type, typename T::difference_type>::value), "");
-    static_assert((std::is_same<typename R::reference, typename T::reference>::value), "");
-    static_assert((std::is_same<typename R::pointer, typename std::iterator_traits<It>::pointer>::value), "");
-
-#if TEST_STD_VER <= 14
-    typedef std::iterator<typename T::iterator_category, typename T::value_type> iterator_base;
-    static_assert((std::is_base_of<iterator_base, R>::value), "");
-#endif
-#if TEST_STD_VER > 17
-    if constexpr (std::is_same_v<typename T::iterator_category, std::contiguous_iterator_tag>) {
-        static_assert((std::is_same<typename R::iterator_category, std::random_access_iterator_tag>::value), "");
-    } else {
-        static_assert((std::is_same<typename R::iterator_category, typename T::iterator_category>::value), "");
-    }
-#else
-    static_assert((std::is_same<typename R::iterator_category, typename T::iterator_category>::value), "");
-#endif
-}
-
-int main(int, char**)
-{
-    test<bidirectional_iterator<char*> >();
-    test<random_access_iterator<char*> >();
-    test<char*>();
-
-#if TEST_STD_VER > 17
-    test<contiguous_iterator<char*>>();
-    static_assert(std::is_same_v<typename std::reverse_iterator<bidirectional_iterator<char*>>::iterator_concept, std::bidirectional_iterator_tag>);
-    static_assert(std::is_same_v<typename std::reverse_iterator<random_access_iterator<char*>>::iterator_concept, std::random_access_iterator_tag>);
-    static_assert(std::is_same_v<typename std::reverse_iterator<contiguous_iterator<char*>>::iterator_concept, std::random_access_iterator_tag>);
-    static_assert(std::is_same_v<typename std::reverse_iterator<char*>::iterator_concept, std::random_access_iterator_tag>);
-#endif
-
-    return 0;
-}