[libcxx] makes `iterator_traits` C++20-aware

[ldionne]

Summary

• moves C++17 iterator_traits
• adds iterator_traits specialisation that supports all expected member aliases except for pointer
• adds iterator_traits specialisations for iterators that meet the legacy iterator requirements but might lack multiple member aliases
• makes pointer iterator_traits specialisation require objects

Depends on D99854.

[Mordante]

libcxx/include/iterator

713

Does this hunk affect the synopsis?

733

For clarity "If I has" -> "If I has".

742

Will __primary_template be used in a later patch?

756

I rather remove the underscores in _`cpp17-input-iterator`_,". I don't think they add clarity. The same for the other places where you used these concepts.

909

Please update the synopsis.

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.pass.cpp

17

I would like some tests using the contiguous_iterator_tag see whether it acts as a random_access_iterator_tag.

184

std::same_as compares the same type.

227

Would it be nice to test whether the test fails when missing_iterator_category is provided but not a typename? E.g.
int missing_iterator_category
And similar for the tests below?

[cjdb]

applies @Mordante's feedback

[cjdb]

libcxx/include/iterator

713

Apparently not. Our synopsis for iterator_traits is far more in-depth than [iterator.synopsis], so I'm not sure if we want to go the extra mile and add it. If anything, I'd prefer to delete some of our header's synopsis to match the standard.

742

Yes (next patch), but it also is a part of the standard to make this the primary template :-)

756

Sure, this isn't Markdown anyway.

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.pass.cpp

17

Is that in scope for this test? I need a bit of convincing :-)

[cjdb]

rebases to activate CI

[cjdb]

rebases to activate CI

[tcanens]

libcxx/include/iterator

736

I think all of these cases need to be routed through a base class template from which iterator_traits derives. "Primary template" means "primary template"; if I specialize iterator_traits for my types, it must not create an ambiguity with these specializations, even if I write my specialization like

template<class I>
    requires is_my_iterator<I>
struct std::iterator_traits<I>{
    // ...
};

[cjdb]

moves iterator_traits into __iterator_traits as per @tcanens' feedback
hopefully gets CI working

[cjdb]

rebases to activate CI

[Mordante]

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.pass.cpp

17

The contiguous_­iterator is part of the cxx20 iterators, so seems like a good reason to test it here. If not, where do you want to test it?

184

I still see the same types ;-)

[zoecarver]

The logic LGTM.

libcxx/include/iterator

743

Can you define this before iterator_traits? I feel like there's needless complexity here.

766

Again, is there any reason we can't define these before we use them?

882

Can you put a comment // !defined... here too?

911

This also needs a few updated: value_type should now remove cv, adds iterator_concept , adds a const specialization. Are you making those changes in another patch?

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp

82 ↗

(On Diff #336695)

I feel like both this and get_pointer are doing the thing where you write the same implementation twice and test that they do the same thing. IMHO that's not a great way to write tests.

107 ↗

(On Diff #336695)

Side note: the functions don't actually return anything useful. All the testing is done when the function is instantiated. So, why is this nodiscard? Wouldn't instantiating this function template inside a function be the same as instantiating it inside a static_assert?

125 ↗

(On Diff #336695)

This indentation seems a bit odd, is this what clang format spit out? Either way, can you un-indent?

162 ↗

(On Diff #336695)

This test I really like because I can read it and immediately see that it's doing the right thing. You say, "here is the input" and "here is the non-computed literal output."

251 ↗

(On Diff #336695)

Nice. This is a good idea to test :)

289 ↗

(On Diff #336695)

I have no idea what the consensus here was... are we still doing compile.passp.cpp tests or no? If no, then we should make this a "normal" .pass test.

libcxx/test/std/iterators/iterator.primitives/iterator.traits/empty.fail.cpp

103

Nit: // _LIBCPP_STD_VER <= 17 || defined(_LIBCPP_HAS_NO_CONCEPTS)

[zoecarver]

libcxx/include/iterator

743

This is a sort of general note for the whole patch. I think it's best to define things and then use them. I don't really see any benefit to declaring them, using them, then defining them. And it makes it harder to read/understand, increases code size, complexity, compile time, etc.

[cjdb]

libcxx/include/iterator

766

C++ best practice is to define things as close as possible to their first point of use. The other specialisations don't need these, and IMO shouldn't be able to see them.

911

value_type should now remove cv,

Already applied (see below).

adds iterator_concept

Already applied (see below).

adds a const specialization

I don't see this in [iterator.traits]?

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp

82 ↗

(On Diff #336695)

This was the best I could come up with. If you're able to think of a different way to get this information, I'm all ears.

289 ↗

(On Diff #336695)

Consensus was to keep .compile.pass.cpp and for them to no longer have main.

[zoecarver]

libcxx/include/iterator

911

Already applied (see below).

Even better :)

I don't see this in [iterator.traits]?

Ah, I said the wrong thing. I meant it removes the const specialization. And I guess we never had that specialization, so nothing to be done.

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp

82 ↗

(On Diff #336695)

I think thinking about it as "getting this information" is not the right way to think about it in the context of a test. The test's job isn't to get that information. It's to verify that the implementation knows how to get that information.

This will require writing this test a bit differently, so maybe before implementing my suggestion (if you're amenable to it), we should check with others (cc @ldionne and @Quuxplusone).

What if you wrote check_wrapper_traits as

template <class I, class Wrapper, class Category, class Pointer, class Reference>
constexpr bool check_wrapper_traits() {
  // ...
  static_assert(std::same_as<typename Traits::reference, Reference>);
  static_assert(std::same_as<typename Traits::pointer, Pointer>);
}

In this example, you'd have to launder Pointer and Reference through the other helper templates, so maybe it's not ideal, but I hope you get what I'm saying (if not, I'll explain more clearly).

Basically, what I'm suggesting is that you give the test the literal expect value that you (as the human) figure out by looking at the standard. Just like what's done in check_with_legacy_iterator.

289 ↗

(On Diff #336695)

Great. I like this idea a lot. Keeping the time it takes to run the tests down :)

[• Quuxplusone]

libcxx/include/iterator

743

Big +1 to what @zoecarver said. More code = more bugs; but even more importantly, complicated code = complicated bugs. Let's make this code as simple as possible. That means as few forward declarations as possible: instead, define entities in the "logical order" so that we build up linearly from the simple details to the big-picture top-level users, with as little "mixing" of levels as possible.

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp

82 ↗

(On Diff #336695)

@zoecarver: I spent a while and wrote up how I'd test all the codepaths here — by following the case-by-case enumeration laid out on cppreference. Here it is:

https://godbolt.org/z/qqKeM3do9

Basically, we expect a special case for pointers; and then we have the simple cases for "all five user-provided" and "four of the five user-provided"; and then we get into all the LegacyFooIterator concepts; and finally we have the fallback to LegacyIterator a.k.a. output-iterator. All of these tests pass with libstdc++ and with MSVC, so I'd hope they pass with libc++'s implementation as well.

I didn't get around to writing the tests for static_assert(isnt_an_iterator_v<T>). I'd probably put those tests in a separate .cpp file, because they'd look very different.

libcxx/test/std/iterators/iterator.primitives/iterator.traits/legacy_iterator_wrappers.h

109–111

This function shouldn't need a body, any more than the other member functions do.
(It's also weird that you're building up these "layered" overload sets of member and friend functions; e.g. a legacy_random_access_iterator will be implicitly convertible to a legacy_iterator whose operator++ has a different return type from the original. Personally I wouldn't use inheritance for this kind of thing; I'd do what "test_iterators.h" does and write completely minimal "archetypes" with no metaprogramming at all. Any chance we could convince you to pursue that minimal direction?)

(EDIT: I've done this now. Here's my suggested tests: https://godbolt.org/z/qqKeM3do9 )

[zoecarver]

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp

82 ↗

(On Diff #336695)

I like these tests a lot. I think this is a perfect example of how we should be testing these things (IMHO). All the cases are covered, and yet, they are super simple and easily verifiable. When I look at something like:

using PT = std::iterator_traits<int*>;
static_assert(std::same_as<PT::iterator_category, std::random_access_iterator_tag>);
static_assert(std::same_as<PT::value_type, int>);
static_assert(std::same_as<PT::difference_type, std::ptrdiff_t>);
static_assert(std::same_as<PT::reference, int&>);
static_assert(std::same_as<PT::pointer, int*>);
static_assert(std::same_as<PT::iterator_concept, std::contiguous_iterator_tag>);

it is immediately clear a) what is being tested, and b) if the test is correct. If you have something computed, like:

static_assert(std::same_as<typename Traits::reference,
                           typename get_reference<I, Wrapper>::type>);

It isn't as clear what iterator is being tested, but more importantly, it isn't clear that the test is correct. I have to go through all the logic of who is calling this function, what the I and Wrapper types are, and then what the implementation logic for get_reference is. All of those are error prone and not really necessary. For tests we shouldn't be testing that the implementation does the same thing as X, we should be testing that the implementation does the right thing.

I would be in support of using those tests instead (or maybe in addition to some of the existing tests). Please update some of the iterator names, though (such as A and B to be more descriptive).

[cjdb]

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp

82 ↗

(On Diff #336695)

Sure, I can adopt it.

by following the case-by-case enumeration laid out on cppreference.

I'm going to have to write this from scratch (or at least thoroughly verify it), because this wasn't derived from the standard draft. I have no way of knowing if (a) cppreference is up-to-date, and (b) if it's correct.

libcxx/test/std/iterators/iterator.primitives/iterator.traits/legacy_iterator_wrappers.h

109–111

Any chance we could convince you to pursue that minimal direction?

Yes, per my comment above. Give me a day or so to work through it please.

[cjdb]

rebases to activate CI

[ldionne]

I like that you're commenting with the bullets, it makes it easy to follow. Generally LGTM with a few issues, but I think we're pretty close already.

libcxx/include/iterator

725

I find that introducing this __has_member_reference intermediate concept increases complexity a bit without adding much. I think it would be fine to just use requires { typename _Ip::reference; } in the two places where it is used. Same for __has_member_iterator_category.

747

For the primary template, pointer should be (http://eel.is/c++draft/iterator.traits#3.1):

If the qualified-id I​::​pointer is valid and denotes a type, then iterator_­traits<I>​::​pointer names that type; otherwise, it names void.

We shouldn't check for operator->(), so using __iterator_traits_member_pointer<_Ip> (which checks for operator->()) doesn't sound correct to me. We should also add a test that catches that: define a type T from which we could deduce iterator_traits<T>::pointer using operator->() but not otherwise, and confirm that iterator_traits<T>::pointer is void. Before fixing this comment, the result would have been whatever's deduced by operator->.

783–793

That way __has_arrow makes sense on its own.

[cjdb]

rebases so @zoecarver can commandeer without issue

[zoecarver]

rebases so @zoecarver can commandeer without issue

Worked like a charm. Thanks!

[zoecarver]

libcxx/include/iterator

747

Yes, @ldionne you're right. Here's an example (from Arthur's godbolt) which catches this:

struct NoPointerMember {
  struct iterator_category {};
  struct value_type {};
  struct difference_type {};
  struct reference {};
  value_type* operator->() const;  // ignored, because B is not a LegacyInputIterator
};
static_assert(std::same_as<NoPointerMemberTraits::pointer, void>);

On both MSVC and GCC this compiles just fine, but we (currently) think the pointer type should be value_type*.

[zoecarver]

Adds Arthur's (modified) tests. I have not started to apply the other review comments yet.

[zoecarver]

• Base diff on correct commit.

[cjdb]

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp

72 ↗

(On Diff #337936)

TODO?

278 ↗

(On Diff #337936)

I think these are a duplication of what's in test/libcxx/iterators/iterator.requirements/iterator.assoc.types/iterator.traits/iterator_traits_cpp17_iterators.h. Could you please confirm?

libcxx/test/std/iterators/iterator.primitives/iterator.traits/empty.fail.cpp

103

This should be !defined(_LIBCPP_HAS_NO_RANGES) now (I didn't get around to fixing it up).

libcxx/test/std/iterators/iterator.primitives/iterator.traits/legacy_iterator_wrappers.h

2

Do we still need this file?

[zoecarver]

...while I update the rest of the comments. (Should have done this last night, sorry.)

[zoecarver]

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp

72 ↗

(On Diff #337936)

To remind me to fix the pointer thing. No longer needed. Thanks.

278 ↗

(On Diff #337936)

Yes, they're very similar. I think the only real difference is the difference_type. This one has a weird difference type, which I think is good because it shows that it's getting it from the correct place.

That being said, this is a fairly small object, and I think it's beneficial to the reader to have the short test next to the short object. It makes it easy to see exactly what the input and output it without having to look for something or think too hard. But that's just my opinion, maybe others feel differently.

libcxx/test/std/iterators/iterator.primitives/iterator.traits/empty.fail.cpp

103

Actually, I don't think so. This is in the std/ directory. If it was in the libc++ directory, I would agree. But ideally these are "generic" standard library tests. They wouldn't rely on any of our implementation details.

[zoecarver]

• Add a few more tests.
• Fix inline comments.
• Re structure impl.
• Remove "-_" characters.

[zoecarver]

• (Again) base on correct commit.

Sorry for the extra email trafic.

[zoecarver]

• Apply Louis' comment about has_arrow

[zoecarver]

Okay, this is now ready for re-review!

[cjdb]

Please also add tests to ensure that all standard library iterators are compatible with iterator_traits. It would be a shame if we introduced a regression.

libcxx/include/iterator

723–728

I know I wrote this, but it'd be better if we used nested requires expressions so the requirements can be congruently ordered with the wording.

740–743

I'd prefer that we forward declare __iterator_traits and then define it under [iterator.traits]/3.3. That's what we did for [[ https://reviews.llvm.org/D96657#inline-913994 | common_reference ]], and I think that helped improve readability. Similarly elsewhere.

774

To distinguish this from member_pointer_or_void we should rename to member_pointer_or_arrow_or_void

831

Typo (scan the comments for other typos such as iteratortraits too).

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp

27–37 ↗

(On Diff #338203)

libcxx/test/std/iterators/iterator.primitives/iterator.traits/pointer.pass.cpp already covers this case.

39–48 ↗

(On Diff #338203)

libcxx/test/std/iterators/iterator.primitives/iterator.traits/const_pointer.pass.cpp has 5/6 of these tests. Please move the iterator_concept case there.

libcxx/test/std/iterators/iterator.primitives/iterator.traits/volatile_pointer.pass.cpp and libcxx/test/std/iterators/iterator.primitives/iterator.traits/const_volatile_pointer.pass.cpp will need updates too.

49 ↗

(On Diff #338203)

It'd be great if you could section off each subset of tests with the paragraph numbers they correspond to.

49–57 ↗

(On Diff #338203)

I think these should be std::same_as<VectorIteratorTraits::member, std::vector<int>::iterator::member>. I don't particularly care about the concrete types they resolve to, but I do care that they match the members of said iterator.

I also notice there's no test for whether or not vector iterators have iterator_concept.

108 ↗

(On Diff #338203)

Please indicate why. It took me a moment to realise this is expected.

126 ↗

(On Diff #338203)

Please also add tests for the minimalistic iterator archetypes in test/libcxx/iterators/iterator.requirements/iterator.assoc.types/iterator.traits/iterator_traits_cpp17_iterators.h.

153 ↗

(On Diff #338203)

I think we should assert that each of these types actually meet/don't meet the cpp17-*-iterator requirements we expect them to. Also, @ldionne asked that the concepts be renamed to __iterator_traits_detail::cpp17_*_iterator, so I think we should reflect that update here.

244 ↗

(On Diff #338203)

I think declaring the operator as deleted would be a good idea.

libcxx/test/std/iterators/iterator.primitives/iterator.traits/empty.fail.cpp

103

In that case, please switch to feature-test macros.

[• Quuxplusone]

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp

49–57 ↗

(On Diff #338203)

I do care what they resolve to. Leave them as-is, i.e., static_assert(std::same_as<VectorIteratorTraits::value_type, int>); is the best way to write it. (Although, if you want to name the typedef VIT to save on column-width and thus fix that one ugly linebreak, that'd be a good idea.)

244 ↗

(On Diff #338203)

No, the point of this test is that the operator is missing (not in overload resolution at all). Anyway, this way is shorter, which is good.

libcxx/test/std/iterators/iterator.primitives/iterator.traits/empty.fail.cpp

103

I guess the real question is, why is this test here at all? Maybe it should move to a different test file, which REQUIRES: c++03 || c++11 || c++14 || c++17.

[zoecarver]

libcxx/include/iterator

740–743

IMHO forward declaring types when we don't have to introduces a lot of complexity and makes the implementation much more difficult to read. Especially if I'm looking for the definition of something later.

I wasn't a reviewer on that patch, but if I were I would have pushed for a different implementation, or at least different names. I find it super confusing to have __common_ref forward declared and then used before being defined. Not to mention the presence of the following names __common_reference_sub_bullet{1,2,3} and __common_ref_{C,D}. When future me is trying to find the implementation of something in libc++, I'd rather not have to do the archeology to decode such names, and having them be forward declared, make it that much more difficult to find the actual implementation.

(Side note: I think we should really discourage naming thing after bullets in the standard. Those change fairly frequently, and the names of things in libc++ hardly ever change. Not only does it make things harder to read/understand, but will likely become incorrect soon.)

However, I agree that this could be made more readable. WDYT about removing __iterator_traits (I don't see any reason we can't just implement iterator_traits directly), and moving all the "implementation details" above so that we can just have one specialization after another. I know this still means we have to put the final "Otherwise" first, but I don't think that adds any complexity, it's the natural way to implement something (I'm sure when creating concepts the committee didn't expect everyone to define the "default" case last using a forward declared class).

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp

49–57 ↗

(On Diff #338203)

I also notice there's no test for whether or not vector iterators have iterator_concept.

I'll add some tests.

I do care what they resolve to. Leave them as-is, i.e., static_assert(std::same_as<VectorIteratorTraits::value_type, int>); is the best way to write it.

I agree. I think it makes more sense to use the concrete types.

(Although, if you want to name the typedef VIT to save on column-width and thus fix that one ugly linebreak, that'd be a good idea.)

I thought clang format wasn't going to generate line breaks anymore...

[zoecarver]

libcxx/include/iterator

723–728

Not sure I follow. Do you want me to make this two requires expressions, or do you want me to add __has_member_{difference_type,value_type}?

[cjdb]

libcxx/include/iterator

740–743

WDYT about removing __iterator_traits (I don't see any reason we can't just implement iterator_traits directly), and moving all the "implementation details" above so that we can just have one specialization after another.

@tcanens made a comment early on in this patch's review pointing out that the primary template mustn't conflict with others' specialisations.

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp

49–57 ↗

(On Diff #338203)

I do care what they resolve to. Leave them as-is, i.e., static_assert(std::same_as<VectorIteratorTraits::value_type, int>); is the best way to write it.

I agree. I think it makes more sense to use the concrete types.

In that case, keep them, and please add the tests I've requested. It's important that VectorIteratorTraits::member is std::vector<int>::iterator::member.

(Although, if you want to name the typedef VIT to save on column-width and thus fix that one ugly linebreak, that'd be a good idea.)

I thought clang format wasn't going to generate line breaks anymore...

Cryptic names hurt readability far more than benign whitespace.

[cjdb]

libcxx/include/iterator

723–728

requires {
  typename _Ip::value_type;
  typename _Ip::difference_type;
  requires __has_member_reference<_Ip>; // this is a nested-requires-expression
  requires __has_member_iterator_category<_Ip>;
};

However, you did point out that the ordering of wording is subject to change, so perhaps this is a moot point to discuss.

[cjdb]

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp

244 ↗

(On Diff #338203)

In that case the type should be renamed to indicate this. "Missing" is ambiguous.

[zoecarver]

libcxx/include/iterator

831

Ah, phab can't display \x{AD}. Great. (Not that we should really be using this character anyway...)

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp

126 ↗

(On Diff #338203)

I'll test iterator_traits_cpp17_iterator, iterator_traits_cpp17_input_iterator, iterator_traits_cpp17_forward_iterator. I think everything else is accounted for.

153 ↗

(On Diff #338203)

Sorry, I'm not sure I understand exactly what you're asking for here. Could you elaborate? Do you want me to update LegacyInputNoValueType and put it in a namespace?

libcxx/test/std/iterators/iterator.primitives/iterator.traits/empty.fail.cpp

103

I guess the real question is, why is this test here at all? Maybe it should move to a different test file, which REQUIRES: c++03 || c++11 || c++14 || c++17.

This seems pretty in line with the other tests in this file. I could argue that we don't need this test to begin with, or that we should be testing this another way, but I won't, because I'm not sure it matters too much :)

In that case, please switch to feature-test macros.

Well, we haven't yet added the __cpp_lib_ranges macro. Was there another macro you were thinking of? I think it's fine to just use _LIBCPP_STD_VER <= 17 || defined(_LIBCPP_HAS_NO_CONCEPTS) because those are already used all over the test suite. To be honest, we could probably use _LIBCPP_HAS_NO_RANGES as well. I was just trying to limit the number of libc++-specific macros we use in "non-libc++" tests.

[cjdb]

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp

153 ↗

(On Diff #338203)

Renaming to s/Legacy/IteratorTraitsCpp17/ should do the trick.

libcxx/test/std/iterators/iterator.primitives/iterator.traits/empty.fail.cpp

103

Was there another macro you were thinking of?

Specifically TEST_STD_VER <= 17 || !defined(__cpp_lib_concepts). That eliminates the implementation-detail names in userspace altogether :)

[zoecarver]

• Apply review comments
• Format
• Move around iterator_traits impl so that all __iterator_traits specializations are in a row.

[zoecarver]

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp

153 ↗

(On Diff #338203)

I don't want to start bike shedding the name, but some of these names get fairly long. I'd really prefer to use "Legacy" especially in the tests. @ldionne wdyt?

[zoecarver]

• Remove internal macros from tests.

[cjdb]

I've pinged comments that I think either have been missed or warrant discussion if you disagree with my commentary.

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp

114 ↗

(On Diff #338607)

Please don't forget std::vector<bool>::iterator and const_iterators (I'll turn a blind eye toward all containers' reverse_iterator members if std::reverse_iterator is tested).

124–131 ↗

(On Diff #338607)

Please don't forget the multis and local_iterators.

133–145 ↗

(On Diff #338607)

Please don't forget std::reverse_iterator, the filesystem iterators, and the regex iterators.

27–37 ↗

(On Diff #338203)

Ping.

39–48 ↗

(On Diff #338203)

Ping.

49 ↗

(On Diff #338203)

Ping.

[ldionne]

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp

153 ↗

(On Diff #338203)

Just to clarify, I asked that the __cpp17_xxx_iterator helper concepts be moved to a namespace after we determined that they were not the same as the Cpp17XXXIterator concepts in the Standard, so as to avoid making them easy to mistake with those concepts.

I don't understand why/how that affects the naming of iterators in the tests. As a result, LegacyXXXIterator like done below currently seems acceptable to me.

[cjdb]

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp

153 ↗

(On Diff #338203)

WFM, I just wanted to bring light to it.

[zoecarver]

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp

27–37 ↗

(On Diff #338203)

Removed.

39–48 ↗

(On Diff #338203)

I think this is done, no? I just haven't removed the tests from here as well.

49 ↗

(On Diff #338203)

I haven't done this yet. This will take some time. I just uploaded a new diff without this change, you can review the rest of the diff, and I'll upload this part when I'm done.

[zoecarver]

• Add all c++ stdlib iterators.
• * Apply remaining review comments.

[ldionne]

This LGTM from my perspective. I think we can go back and polish some of the naming (honestly I think names like member_pointer_or_arrow_or_void are difficult to read and reason about), however I don't want to hold up this important review which blocks several dependencies from making progress because of that.

This LGTM and I think we should ship this as-is unless a reviewer has strong concerns that have not been alleviated yet.

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp

49 ↗

(On Diff #338203)

IMO, I'm not sure the cost/benefit ratio is sufficient to do it. At least it's definitely not blocking.

[zoecarver]

• Add a test for regex_token_iterator.

[zoecarver]

I will be committing when the CI passes.

[zoecarver]

• Remove test for iterator_traits<void*> pre-C++17. void can't be referenced so this was failing with a hard error.

[ldionne]

Commandeering to fix CI.

[ldionne]

Try to fix CI.

libcxx/include/iterator

struct iterator_traits
     typedef typename Iterator::difference_type difference_type;
     typedef typename Iterator::value_type value_type;
     typedef typename Iterator::pointer pointer;
     typedef typename Iterator::reference reference;
     typedef typename Iterator::iterator_category iterator_category;
 };
 
 template<class T>
+  requires is_object_v<T>                    // since C++20
 struct iterator_traits<T*>
 {
     typedef ptrdiff_t difference_type;
     typedef T value_type;
     typedef T* pointer;
     typedef T& reference;
     typedef random_access_iterator_tag iterator_category;
 };
requires(_Ip __i, _Ip __j, typename incrementable_traits<_Ip>::difference_type __n) {
     { __i += __n } -> same_as<_Ip&>;
     { __i -= __n } -> same_as<_Ip&>;
     { __i +  __n } -> same_as<_Ip>;
     { __n +  __i } -> same_as<_Ip>;
     { __i -  __n } -> same_as<_Ip>;
     { __i -  __j } -> same_as<decltype(__n)>;
     {  __i[__n]  } -> convertible_to<iter_reference_t<_Ip>>;
   };
-#endif // _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_CONCEPTS)
 
+template<class> struct __iterator_traits_member_pointer;
+template<class> struct __iterator_traits_member_reference;
+template<class> struct __iterator_traits_iterator_category;
+
+template<class _Ip>
+concept __has_member_reference = requires { typename _Ip::reference; };
+
+template<class _Ip>
+concept __has_member_iterator_category = requires { typename _Ip::iterator_category; };
+
+template<class _Ip>
+concept __specifies_members =
+  __has_member_reference<_Ip> &&
+  __has_member_iterator_category<_Ip> &&
+  requires {
+    typename _Ip::difference_type;
+    typename _Ip::value_type;
+  };
+
+// [iterator.traits]/3.1
+// If `I` has valid ([temp.deduct]) member types `difference_­type`, `value_­type`, `reference`, and
+// `iterator_­category`, then `iterator_­traits<I>` has the following publicly accessible members:
+template<__specifies_members _Ip>
+struct _LIBCPP_TEMPLATE_VIS iterator_traits<_Ip> {
+  using iterator_category  = typename _Ip::iterator_category;
+  using value_type         = typename _Ip::value_type;
+  using difference_type    = typename _Ip::difference_type;
+  using pointer            = typename __iterator_traits_member_pointer<_Ip>::type;
+  using reference          = typename _Ip::reference;
+  using __primary_template = iterator_traits;
+};
+
+template<class _Tp>
+concept __legacy_iterator_missing_members =
+  !__specifies_members<_Tp> &&
+  __legacy_iterator<_Tp>;
+
+template<class _Tp>
+concept __legacy_input_iterator_missing_members =
+  __legacy_iterator_missing_members<_Tp> &&
+  __legacy_input_iterator<_Tp>;
+
+// [iterator.traits]/3.2
+// Otherwise, if `I` satisfies the exposition-only concept `cpp17-input-iterator`,
+// `iterator_­traits<I>` has the following publicly accessible members:
+template<__legacy_input_iterator_missing_members _Ip>
+struct _LIBCPP_TEMPLATE_VIS iterator_traits<_Ip> {
+  using iterator_category = typename __iterator_traits_iterator_category<_Ip>::type;
+  using value_type        = typename indirectly_readable_traits<_Ip>::value_type;
+  using difference_type   = typename incrementable_traits<_Ip>::difference_type;
+  using pointer           = typename __iterator_traits_member_pointer<_Ip>::type;
+  using reference         = typename __iterator_traits_member_reference<_Ip>::type;
+  using __primary_template = iterator_traits;
+};
+
+// [iterator.traits]/3.2.1
+// If the qualified-id `I::pointer` is valid and denotes a type, `pointer` names that type.
+template<class _Ip>
+concept __has_member_pointer = requires { typename _Ip::pointer; };
+
+template<__has_member_pointer _Ip>
+struct __iterator_traits_member_pointer<_Ip> { using type = typename _Ip::pointer; };
+
+// Otherwise, if `decltype(declval<I&>().operator->())` is well-formed, then `pointer` names that
+// type.
+template<class _Ip>
+concept __has_arrow =
+  __legacy_input_iterator_missing_members<_Ip> &&
+  !__has_member_pointer<_Ip> &&
+  requires(_Ip& __i) {
+    __i.operator->();
+  };
+
+template<__has_arrow _Ip>
+struct __iterator_traits_member_pointer<_Ip> {
+  using type = decltype(declval<_Ip&>().operator->());
+};
+
+// Otherwise, `pointer` names `void`.
+template<class>
+struct __iterator_traits_member_pointer { using type = void; };
+
+// [iterator.traits]/3.2.2
+// If the qualified-id `I::reference` is valid and denotes a type, `reference` names that type.
+template<__has_member_reference _Ip>
+struct __iterator_traits_member_reference<_Ip> { using type = typename _Ip::reference; };
+
+// Otherwise, `reference` names `iter_­reference_­t<I>`.
+template<class _Ip>
+struct __iterator_traits_member_reference { using type = iter_reference_t<_Ip>; };
+
+// [iterator.traits]/3.2.3
+// If the qualified-id `I::iterator_­category` is valid and denotes a type, `iterator_­category` names
+// that type.
+template<__has_member_iterator_category _Ip>
+struct __iterator_traits_iterator_category<_Ip> {
+  using type = typename _Ip::iterator_category;
+};
+
+// Otherwise, iterator_­category names:
+template<class>
+struct __deduce_iterator_category;
+
+template<class _Ip>
+struct __iterator_traits_iterator_category : __deduce_iterator_category<_Ip> {};
+
+// [iterator.traits]/3.2.3.1
+// `random_­access_­iterator_­tag` if `I` satisfies `cpp17-random-access-iterator`, or otherwise
+template<__legacy_random_access_iterator _Ip>
+struct __deduce_iterator_category<_Ip> {
+  using type = random_access_iterator_tag;
+};
+
+// [iterator.traits]/3.2.3.2
+// `bidirectional_­iterator_­tag` if `I` satisfies `cpp17-bidirectional-iterator`, or otherwise
+template<__legacy_bidirectional_iterator _Ip>
+struct __deduce_iterator_category<_Ip> {
+  using type = bidirectional_iterator_tag;
+};
+
+// [iterator.traits]/3.2.3.3
+// `forward_­iterator_­tag` if `I` satisfies `cpp17-forward-iterator`, or otherwise
+template<__legacy_forward_iterator _Ip>
+struct __deduce_iterator_category<_Ip> {
+  using type = forward_iterator_tag;
+};
+
+// [iterator.traits]/3.2.3.4
+// input_­iterator_­tag
+template<class _Ip>
+struct __deduce_iterator_category {
+  using type = input_iterator_tag;
+};
+
+
+// [iterator.traits]/3.3
+template<class> struct __iterator_traits_difference_type;
+
+// Otherwise, if `I` satisfies the exposition-only concept `cpp17-iterator`, then
+// `iterator_­traits<I>` has the following publicly accessible members:
+template<__legacy_iterator_missing_members _Ip>
+struct _LIBCPP_TEMPLATE_VIS iterator_traits<_Ip> {
+  using iterator_category = output_iterator_tag;
+  using value_type        = void;
+  using difference_type   = typename __iterator_traits_difference_type<_Ip>::type;
+  using pointer           = void;
+  using reference         = void;
+  using __primary_template = iterator_traits;
+};
+
+// If the qualified-id `incrementable_­traits<I>::difference_­type` is valid and denotes a type, then
+// `difference_­type` names that type;
+template<class _Ip>
+requires requires { typename incrementable_traits<_Ip>::difference_type; }
+struct __iterator_traits_difference_type<_Ip> {
+  using type = typename incrementable_traits<_Ip>::difference_type;
+};
+
+// otherwise, it names void.
+template<class>
+struct __iterator_traits_difference_type { using type = void; };
+
+// [iterator.traits]/3.4
+// Otherwise, `iterator_­traits<I>` has no members by any of the above names.
+template<class>
+struct _LIBCPP_TEMPLATE_VIS iterator_traits {
+  using __primary_template = iterator_traits;
+};
+#else
 template <class _Iter, bool> struct __iterator_traits {};
 
 template <class _Iter>
 struct __iterator_traits<_Iter, true>
     :  __iterator_traits_impl
       <
         _Iter,
         is_convertible<typename _Iter::iterator_category, input_iterator_tag>::value ||
         is_convertible<typename _Iter::iterator_category, output_iterator_tag>::value
       >
 {};
 
 // iterator_traits<Iterator> will only have the nested types if Iterator::iterator_category
 //    exists.  Else iterator_traits<Iterator> will be an empty class.  This is a
 //    conforming extension which allows some programs to compile and behave as
 //    the client expects instead of failing at compile time.
 
 template <class _Iter>
 struct _LIBCPP_TEMPLATE_VIS iterator_traits
     : __iterator_traits<_Iter, __has_iterator_typedefs<_Iter>::value> {
 
   using __primary_template = iterator_traits;
 };
+#endif // _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_CONCEPTS)
 
 template<class _Tp>
+#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_CONCEPTS)
+requires is_object_v<_Tp>
+#endif
 struct _LIBCPP_TEMPLATE_VIS iterator_traits<_Tp*>
 {
     typedef ptrdiff_t difference_type;
     typedef typename remove_cv<_Tp>::type value_type;
     typedef _Tp* pointer;
     typedef _Tp& reference;
     typedef random_access_iterator_tag iterator_category;
 #if _LIBCPP_STD_VER > 17
     typedef contiguous_iterator_tag    iterator_concept;

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.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
+// UNSUPPORTED: gcc-10
+
+// template<class T>
+// struct iterator_traits;
+
+#include <iterator>
+
+#include <concepts>
+#include <vector>
+
+#include "legacy_iterator_wrappers.h"
+
+template <class Category, class Value, class Difference, class Reference>
+struct explicit_members {
+  using iterator_category = Category;
+  using value_type = Value;
+  using difference_type = Difference;
+  using reference = Reference;
+};
+
+template <class Category, class Value, class Difference, class Reference>
+struct explicit_members_with_pointer
+    : explicit_members<Category, Value, Difference, Reference> {
+  using pointer = typename explicit_members<Category, Value, Difference,
+                                            Reference>::value_type*;
+};
+
+template <class Category, class Value, class Difference, class Reference>
+[[nodiscard]] constexpr bool check_members_explicitly_provided() {
+  {
+    using fake_iterator =
+        explicit_members<Category, Value, Difference, Reference>;
+    using iterator_traits = std::iterator_traits<fake_iterator>;
+    static_assert(
+        std::same_as<typename iterator_traits::iterator_category, Category>);
+    static_assert(std::same_as<typename iterator_traits::value_type, Value>);
+    static_assert(
+        std::same_as<typename iterator_traits::difference_type, Difference>);
+    static_assert(std::same_as<typename iterator_traits::reference, Reference>);
+    static_assert(std::same_as<typename iterator_traits::pointer, void>);
+  }
+
+  {
+    using fake_iterator =
+        explicit_members_with_pointer<Category, Value, Difference, Reference>;
+    using iterator_traits = std::iterator_traits<fake_iterator>;
+    static_assert(
+        std::same_as<typename iterator_traits::iterator_category, Category>);
+    static_assert(std::same_as<typename iterator_traits::value_type, Value>);
+    static_assert(
+        std::same_as<typename iterator_traits::difference_type, Difference>);
+    static_assert(std::same_as<typename iterator_traits::reference, Reference>);
+    static_assert(std::same_as<typename iterator_traits::pointer, Value*>);
+  }
+  return true;
+}
+
+static_assert(check_members_explicitly_provided<std::input_iterator_tag, int,
+                                                int, int&>());
+static_assert(check_members_explicitly_provided<std::input_iterator_tag, int,
+                                                long, int&>());
+static_assert(check_members_explicitly_provided<std::forward_iterator_tag, int,
+                                                double, int&>());
+
+struct S {};
+static_assert(check_members_explicitly_provided<std::bidirectional_iterator_tag,
+                                                S, int, long long>());
+static_assert(check_members_explicitly_provided<std::random_access_iterator_tag,
+                                                S, int, long long>());
+
+template <class I, class Wrapper>
+struct get_reference {
+  using type = std::iter_reference_t<Wrapper>;
+};
+
+template <class I, std::derived_from<reference_extension<I> > Wrapper>
+struct get_reference<I, Wrapper> {
+  using type = typename Wrapper::reference;
+};
+
+template <class, class>
+struct get_pointer {
+  using type = void;
+};
+
+template <class I, std::derived_from<pointer_extension<I> > Wrapper>
+struct get_pointer<I, Wrapper> {
+  using type = typename Wrapper::pointer;
+};
+
+template <class I, has_arrow Wrapper>
+struct get_pointer<I, Wrapper> {
+  using type = decltype(std::declval<I&>().operator->());
+};
+
+template <class I, class Wrapper, class Category>
+[[nodiscard]] constexpr bool check_wrapper_traits() {
+  using Traits = std::iterator_traits<Wrapper>;
+  static_assert(std::same_as<typename Traits::iterator_category, Category>);
+  static_assert(
+      std::same_as<typename Traits::value_type,
+                   typename std::indirectly_readable_traits<I>::value_type>);
+  static_assert(
+      std::same_as<typename Traits::difference_type,
+                   typename std::incrementable_traits<I>::difference_type>);
+  static_assert(std::same_as<typename Traits::reference,
+                             typename get_reference<I, Wrapper>::type>);
+  static_assert(std::same_as<typename Traits::pointer,
+                             typename get_pointer<I, Wrapper>::type>);
+  return true;
+}
+
+template <class I, template <typename> class... Args>
+requires(sizeof...(Args) > 0) constexpr
+    void check_with_legacy_input_iterator() {
+  {
+    using iterator = legacy_input_iterator<I, minimal_extension, Args...>;
+    static_assert(check_wrapper_traits<I, iterator, std::input_iterator_tag>());
+  }
+  {
+    using iterator = legacy_forward_iterator<I, minimal_extension, Args...>;
+    static_assert(
+        check_wrapper_traits<I, iterator, std::forward_iterator_tag>());
+  }
+  {
+    using iterator =
+        legacy_bidirectional_iterator<I, minimal_extension, Args...>;
+    static_assert(
+        check_wrapper_traits<I, iterator, std::bidirectional_iterator_tag>());
+  }
+  {
+    using iterator =
+        legacy_random_access_iterator<I, minimal_extension, Args...>;
+    static_assert(
+        check_wrapper_traits<I, iterator, std::random_access_iterator_tag>());
+  }
+}
+
+template <class I>
+[[nodiscard]] constexpr bool check_with_legacy_input_iterator() {
+  check_with_legacy_input_iterator<I, empty_extension>();
+  check_with_legacy_input_iterator<I, reference_extension>();
+  check_with_legacy_input_iterator<I, pointer_extension>();
+  check_with_legacy_input_iterator<I, arrow_extension>();
+  check_with_legacy_input_iterator<I, reference_extension, pointer_extension>();
+  check_with_legacy_input_iterator<I, reference_extension, arrow_extension>();
+  return true;
+}
+static_assert(check_with_legacy_input_iterator<std::vector<int>::iterator>());
+static_assert(
+    check_with_legacy_input_iterator<std::vector<int>::const_iterator>());
+
+template <class I>
+[[nodiscard]] constexpr bool check_with_legacy_iterator() {
+  {
+    using iterator = legacy_iterator<I>;
+    using iterator_traits = std::iterator_traits<iterator>;
+    static_assert(std::same_as<typename iterator_traits::iterator_category,
+                               std::output_iterator_tag>);
+    static_assert(std::same_as<typename iterator_traits::value_type, void>);
+    static_assert(
+        std::same_as<typename iterator_traits::difference_type, void>);
+    static_assert(std::same_as<typename iterator_traits::reference, void>);
+    static_assert(std::same_as<typename iterator_traits::pointer, void>);
+  }
+  {
+    using iterator = legacy_iterator<I, std::incrementable_traits>;
+    using iterator_traits = std::iterator_traits<iterator>;
+    static_assert(std::same_as<typename iterator_traits::iterator_category,
+                               std::output_iterator_tag>);
+    static_assert(std::same_as<typename iterator_traits::value_type, void>);
+    static_assert(std::same_as<typename iterator_traits::difference_type,
+                               typename iterator::difference_type>);
+    static_assert(std::same_as<typename iterator_traits::reference, void>);
+    static_assert(std::same_as<typename iterator_traits::pointer, void>);
+  }
+  return true;
+}
+
+static_assert(check_with_legacy_iterator<std::vector<int>::iterator>());
+static_assert(check_with_legacy_iterator<std::vector<int>::const_iterator>());
+
+template <typename T>
+[[nodiscard]] constexpr bool check_fails() {
+  static_assert(
+      !requires { typename std::iterator_traits<T>::iterator_concept; });
+  static_assert(
+      !requires { typename std::iterator_traits<T>::iterator_category; });
+  static_assert(!requires { typename std::iterator_traits<T>::value_type; });
+  static_assert(
+      !requires { typename std::iterator_traits<T>::difference_type; });
+  static_assert(!requires { typename std::iterator_traits<T>::pointer; });
+  static_assert(!requires { typename std::iterator_traits<T>::reference; });
+  return true;
+}
+
+static_assert(check_fails<void>());
+static_assert(check_fails<void*>());
+static_assert(check_fails<int[]>());
+static_assert(check_fails<int const[]>());
+static_assert(check_fails<double[]>());
+
+static_assert(check_fails<S>());
+static_assert(check_fails<S&>());
+static_assert(check_fails<S* const>());
+static_assert(check_fails<S const&>());
+static_assert(check_fails<S (*)()>());
+static_assert(check_fails<S (&)()>());
+static_assert(check_fails<S[]>());
+
+struct missing_iterator_category {
+  using value_type = int;
+  using difference_type = int;
+  using pointer = int*;
+  using reference = int&;
+};
+static_assert(check_fails<missing_iterator_category>());
+
+struct iterator_category_as_static_member {
+  using value_type = int;
+  using difference_type = int;
+  using pointer = int*;
+  using reference = int&;
+
+  inline static constexpr int iterator_category = 0;
+};
+static_assert(check_fails<iterator_category_as_static_member>());
+
+struct missing_value_type {
+  using iterator_category = std::input_iterator_tag;
+  using difference_type = int;
+  using pointer = int*;
+  using reference = int&;
+};
+static_assert(check_fails<missing_value_type>());
+
+struct value_type_as_static_member {
+  using iterator_category = std::input_iterator_tag;
+  using difference_type = int;
+  using pointer = int*;
+  using reference = int&;
+
+  inline static constexpr int value_type = 0;
+};
+static_assert(check_fails<value_type_as_static_member>());
+
+struct missing_difference_type {
+  using iterator_category = std::input_iterator_tag;
+  using value_type = int;
+  using pointer = int*;
+  using reference = int&;
+};
+static_assert(check_fails<missing_difference_type>());
+
+struct difference_type_as_static_member {
+  using iterator_category = std::input_iterator_tag;
+  using value_type = int;
+  using pointer = int*;
+  using reference = int&;
+
+  inline static constexpr int difference_type = 0;
+};
+static_assert(check_fails<difference_type_as_static_member>());
+
+struct missing_reference {
+  using iterator_category = std::input_iterator_tag;
+  using value_type = int;
+  using difference_type = int;
+  using pointer = int*;
+};
+static_assert(check_fails<missing_reference>());
+
+struct reference_as_static_member {
+  using iterator_category = std::input_iterator_tag;
+  using value_type = int;
+  using difference_type = int;
+  using pointer = int*;
+
+  inline static constexpr int reference = 0;
+};
+static_assert(check_fails<reference_as_static_member>());
+
+int main(int, char**) { return 0; }

libcxx/test/std/iterators/iterator.primitives/iterator.traits/empty.fail.cpp

int main(int, char**)
     {
     typedef std::iterator_traits<NotAnIteratorNoValue> T;
     typedef T::difference_type   DT; // expected-error-re {{no type named 'difference_type' in 'std:{{.*}}:iterator_traits<{{.+}}>}}
     typedef T::value_type        VT; // expected-error-re {{no type named 'value_type' in 'std:{{.*}}:iterator_traits<{{.+}}>}}
     typedef T::pointer           PT; // expected-error-re {{no type named 'pointer' in 'std:{{.*}}:iterator_traits<{{.+}}>}}
     typedef T::reference         RT; // expected-error-re {{no type named 'reference' in 'std:{{.*}}:iterator_traits<{{.+}}>}}
     typedef T::iterator_category CT; // expected-error-re {{no type named 'iterator_category' in 'std:{{.*}}:iterator_traits<{{.+}}>}}
     }
-
+#if _LIBCPP_STD_VER <= 17 || defined(_LIBCPP_HAS_NO_CONCEPTS)
     {
     typedef std::iterator_traits<NotAnIteratorNoPointer> T;
     typedef T::difference_type   DT; // expected-error-re {{no type named 'difference_type' in 'std:{{.*}}:iterator_traits<{{.+}}>}}
     typedef T::value_type        VT; // expected-error-re {{no type named 'value_type' in 'std:{{.*}}:iterator_traits<{{.+}}>}}
     typedef T::pointer           PT; // expected-error-re {{no type named 'pointer' in 'std:{{.*}}:iterator_traits<{{.+}}>}}
     typedef T::reference         RT; // expected-error-re {{no type named 'reference' in 'std:{{.*}}:iterator_traits<{{.+}}>}}
     typedef T::iterator_category CT; // expected-error-re {{no type named 'iterator_category' in 'std:{{.*}}:iterator_traits<{{.+}}>}}
     }
-
+#endif
     {
     typedef std::iterator_traits<NotAnIteratorNoReference> T;
     typedef T::difference_type   DT; // expected-error-re {{no type named 'difference_type' in 'std:{{.*}}:iterator_traits<{{.+}}>}}
     typedef T::value_type        VT; // expected-error-re {{no type named 'value_type' in 'std:{{.*}}:iterator_traits<{{.+}}>}}
     typedef T::pointer           PT; // expected-error-re {{no type named 'pointer' in 'std:{{.*}}:iterator_traits<{{.+}}>}}
     typedef T::reference         RT; // expected-error-re {{no type named 'reference' in 'std:{{.*}}:iterator_traits<{{.+}}>}}
     typedef T::iterator_category CT; // expected-error-re {{no type named 'iterator_category' in 'std:{{.*}}:iterator_traits<{{.+}}>}}
     }

libcxx/test/std/iterators/iterator.primitives/iterator.traits/empty.pass.cpp

 public:
     static const bool value = sizeof(test<T>(0)) == 1;
 };
 
 int main(int, char**)
 {
     typedef std::iterator_traits<not_an_iterator> It;
     static_assert(!(has_value_type<It>::value), "");
+#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_CONCEPTS)
+    static_assert(!(has_value_type<std::iterator_traits<void*> >::value));
+#endif
 
     return 0;
 }

libcxx/test/std/iterators/iterator.primitives/iterator.traits/legacy_iterator_wrappers.h

+//===----------------------------------------------------------------------===//
+//
+// 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 STD_ITERATORS_ITERATOR_PRIMITIVES_ITERATOR_TRAITS_LEGACY_ITERATORS_H
+#define STD_ITERATORS_ITERATOR_PRIMITIVES_ITERATOR_TRAITS_LEGACY_ITERATORS_H
+
+template <class I>
+concept has_arrow = requires(I& i) {
+  i.operator->();
+};
+
+// Extension point for `legacy_iterator_wrapper` to support `operator->`.
+template <has_arrow I>
+struct arrow_extension : I {
+  using I::operator->;
+};
+
+// Wrapper around a legacy C++ iterator to give it a minimal interface.
+//
+// The key difference between these `legacy.*_iterator` types and those found in
+// `support/test_iterators.h` is that the legacy iterator class are designed to support mixins and
+// specifically test C++20's changes to `iterator_traits`. Namely, the absence of members and the
+// arrow operator is important.
+template <class I, template <typename> class... Args>
+class legacy_iterator : public Args<I>... {
+public:
+  legacy_iterator() = default;
+  explicit legacy_iterator(I i) : base_(i) {}
+
+  decltype(auto) operator*() const { return *base_; }
+
+  legacy_iterator& operator++();
+  legacy_iterator operator++(int);
+
+private:
+  I base_ = I();
+};
+
+// Wrapper around a legacy C++ input iterator to give it a minimal interface.
+template <class I, template <typename> class... Args>
+class legacy_input_iterator : public legacy_iterator<I, Args...> {
+public:
+  using iterator_category = std::input_iterator_tag;
+  using legacy_iterator<I, Args...>::legacy_iterator;
+
+  static_assert((requires { typename Args<I>::value_type; } || ...));
+  static_assert((requires { typename Args<I>::difference_type; } || ...));
+
+  bool operator==(legacy_input_iterator const&) const;
+  legacy_input_iterator& operator++();
+  legacy_input_iterator operator++(int);
+};
+
+// Wrapper around a legacy C++ forward iterator to give it a minimal interface.
+template <class I, template <typename> class... Args>
+class legacy_forward_iterator : public legacy_input_iterator<I, Args...> {
+public:
+  using iterator_category = std::forward_iterator_tag;
+  using legacy_input_iterator<I, Args...>::legacy_input_iterator;
+
+  legacy_forward_iterator& operator++();
+  legacy_forward_iterator operator++(int);
+};
+
+// Wrapper around a legacy C++ bidirectional iterator to give it a minimal interface.
+template <class I, template <typename> class... Args>
+class legacy_bidirectional_iterator
+    : public legacy_forward_iterator<I, Args...> {
+public:
+  using iterator_category = std::bidirectional_iterator_tag;
+  using legacy_forward_iterator<I, Args...>::legacy_forward_iterator;
+
+  legacy_bidirectional_iterator& operator++();
+  legacy_bidirectional_iterator operator++(int);
+  legacy_bidirectional_iterator& operator--();
+  legacy_bidirectional_iterator operator--(int);
+};
+
+// Wrapper around a legacy C++ random-access iterator to give it a minimal interface.
+template <class I, template <typename> class... Args>
+class legacy_random_access_iterator
+    : public legacy_bidirectional_iterator<I, Args...> {
+public:
+  using iterator_category = std::random_access_iterator_tag;
+  using legacy_bidirectional_iterator<I,
+                                      Args...>::legacy_bidirectional_iterator;
+  using typename legacy_bidirectional_iterator<I, Args...>::difference_type;
+
+  legacy_random_access_iterator& operator++();
+  legacy_random_access_iterator operator++(int);
+  legacy_random_access_iterator& operator--();
+  legacy_random_access_iterator operator--(int);
+  legacy_random_access_iterator& operator+=(difference_type);
+  legacy_random_access_iterator& operator-=(difference_type);
+
+  friend legacy_random_access_iterator operator+(legacy_random_access_iterator,
+                                                 difference_type);
+  friend legacy_random_access_iterator operator+(difference_type,
+                                                 legacy_random_access_iterator);
+  friend legacy_random_access_iterator operator-(legacy_random_access_iterator,
+                                                 difference_type);
+
+  difference_type operator-(legacy_random_access_iterator) const;
+  decltype(auto) operator[](difference_type const n) const {
+    return *(*this + n);
+  }
+  bool operator<(legacy_random_access_iterator) const;
+  bool operator>(legacy_random_access_iterator) const;
+  bool operator<=(legacy_random_access_iterator) const;
+  bool operator>=(legacy_random_access_iterator) const;
+};
+
+// Minimum extension point for `legacy_input_iterator`.
+template <class I>
+struct minimal_extension {
+  using difference_type =
+      typename std::incrementable_traits<I>::difference_type;
+  using value_type = typename std::indirectly_readable_traits<I>::value_type;
+};
+
+// Extension point for reference member.
+template <class I>
+struct reference_extension {
+  using reference = typename I::reference;
+};
+
+// Extension point for pointer member.
+template <typename I>
+struct pointer_extension {
+  using pointer = typename I::pointer;
+};
+
+// Extension point to indicate no extension points.
+// This is used for function overloading when there isn't a better name for two overloads.
+template <class I>
+struct empty_extension {};
+
+#endif // STD_ITERATORS_ITERATOR_PRIMITIVES_ITERATOR_TRAITS_LEGACY_ITERATORS_H