This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ldionne on Apr 3 2021, 10:41 PM.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Mordante added inline comments.Apr 4 2021, 1:35 AM
libcxx/include/iterator
767

Does this hunk affect the synopsis?

787

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

796

Will __primary_template be used in a later patch?

806

Please update the synopsis.

810

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.

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.pass.cpp
16 ↗(On Diff #335133)

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

183 ↗(On Diff #335133)

std::same_as compares the same type.

226 ↗(On Diff #335133)

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 updated this revision to Diff 335174.Apr 4 2021, 2:19 PM
cjdb marked 5 inline comments as done.

applies @Mordante's feedback

cjdb added inline comments.Apr 4 2021, 4:39 PM
libcxx/include/iterator
767

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.

796

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

810

Sure, this isn't Markdown anyway.

libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.pass.cpp
16 ↗(On Diff #335133)

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

cjdb updated this revision to Diff 335206.Apr 4 2021, 11:17 PM

rebases to activate CI

cjdb updated this revision to Diff 335720.Apr 6 2021, 10:01 PM

rebases to activate CI

tcanens added a subscriber: tcanens.Apr 7 2021, 6:51 PM
tcanens added inline comments.
libcxx/include/iterator
790

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 updated this revision to Diff 335990.Apr 7 2021, 8:58 PM

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

cjdb updated this revision to Diff 336695.Apr 11 2021, 2:35 PM
cjdb marked an inline comment as done.

rebases to activate CI

Mordante added inline comments.Apr 13 2021, 11:07 AM
libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.pass.cpp
16 ↗(On Diff #335133)

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?

183 ↗(On Diff #335133)

I still see the same types ;-)

The logic LGTM.

libcxx/include/iterator
797

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

808

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?

820

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

936

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

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

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.

108

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?

126

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

163

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."

252

Nice. This is a good idea to test :)

290

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 added inline comments.Apr 13 2021, 2:01 PM
libcxx/include/iterator
797

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 added inline comments.Apr 13 2021, 2:15 PM
libcxx/include/iterator
808

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]?

820

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.

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

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.

290

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

zoecarver added inline comments.
libcxx/include/iterator
808

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
83

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.

290

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

libcxx/include/iterator
797

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
83

@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
108–110 ↗(On Diff #336695)

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 added inline comments.Apr 14 2021, 12:23 PM
libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp
83

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 added inline comments.Apr 14 2021, 12:52 PM
libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp
83

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
108–110 ↗(On Diff #336695)

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 updated this revision to Diff 337805.Apr 15 2021, 9:53 AM

rebases to activate CI

ldionne requested changes to this revision.Apr 15 2021, 11:57 AM

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
779

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.

801

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->.

837–847

That way __has_arrow makes sense on its own.

This revision now requires changes to proceed.Apr 15 2021, 11:57 AM
zoecarver commandeered this revision.Apr 15 2021, 2:48 PM
zoecarver edited reviewers, added: cjdb; removed: zoecarver.
cjdb commandeered this revision.Apr 15 2021, 2:53 PM
cjdb edited reviewers, added: zoecarver; removed: cjdb.
cjdb updated this revision to Diff 337913.Apr 15 2021, 2:54 PM

rebases so @zoecarver can commandeer without issue

zoecarver commandeered this revision.Apr 15 2021, 2:55 PM
zoecarver edited reviewers, added: cjdb; removed: zoecarver.

rebases so @zoecarver can commandeer without issue

Worked like a charm. Thanks!

zoecarver added inline comments.Apr 15 2021, 3:57 PM
libcxx/include/iterator
801

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 updated this revision to Diff 337934.Apr 15 2021, 4:23 PM

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

zoecarver updated this revision to Diff 337936.Apr 15 2021, 4:25 PM
  • Base diff on correct commit.
cjdb added inline comments.Apr 15 2021, 4:56 PM
libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp
73

TODO?

279

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
1 ↗(On Diff #337936)

Do we still need this file?

zoecarver planned changes to this revision.Apr 16 2021, 8:40 AM

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

zoecarver added inline comments.Apr 16 2021, 11:59 AM
libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp
73

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

279

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.

  • Add a few more tests.
  • Fix inline comments.
  • Re structure impl.
  • Remove "-_" characters.
  • (Again) base on correct commit.

Sorry for the extra email trafic.

  • Apply Louis' comment about has_arrow

Okay, this is now ready for re-review!

cjdb requested changes to this revision.Apr 18 2021, 3:38 PM

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
611–616

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.

628–631

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.

662

To distinguish this from member_pointer_or_void we should rename to member_pointer_or_arrow_or_void

719

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
28–38

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

40–49

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.

50

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

50–58

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.

109

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

127

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.

154

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.

245

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.

This revision now requires changes to proceed.Apr 18 2021, 3:38 PM
libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp
50–58

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.)

245

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 added inline comments.Apr 19 2021, 9:01 AM
libcxx/include/iterator
628–631

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
50–58

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 added inline comments.Apr 19 2021, 9:08 AM
libcxx/include/iterator
611–616

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 added inline comments.Apr 19 2021, 9:11 AM
libcxx/include/iterator
628–631

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
50–58

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 added inline comments.Apr 19 2021, 9:15 AM
libcxx/include/iterator
611–616
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 added inline comments.Apr 19 2021, 9:16 AM
libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp
245

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

zoecarver marked an inline comment as done.Apr 19 2021, 10:14 AM
zoecarver added inline comments.
libcxx/include/iterator
719

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
127

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

154

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 added inline comments.Apr 19 2021, 10:19 AM
libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp
154

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 :)

  • Apply review comments
  • Format
  • Move around iterator_traits impl so that all __iterator_traits specializations are in a row.
zoecarver added inline comments.Apr 19 2021, 12:49 PM
libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp
154

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?

  • Remove internal macros from tests.
cjdb added a comment.Apr 19 2021, 12:55 PM

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
28–38

Ping.

40–49

Ping.

50

Ping.

114

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

Please don't forget the multis and local_iterators.

133–145

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

ldionne added inline comments.Apr 19 2021, 12:57 PM
libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp
154

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 added inline comments.Apr 19 2021, 12:58 PM
libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp
154

WFM, I just wanted to bring light to it.

zoecarver added inline comments.Apr 19 2021, 1:48 PM
libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp
28–38

Removed.

40–49

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

50

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 updated this revision to Diff 338630.Apr 19 2021, 1:51 PM
  • Add all c++ stdlib iterators.
  • * Apply remaining review comments.
ldionne accepted this revision.Apr 19 2021, 2:05 PM

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
50

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

cjdb accepted this revision.Apr 19 2021, 2:10 PM
This revision is now accepted and ready to land.Apr 19 2021, 2:10 PM
zoecarver updated this revision to Diff 338642.Apr 19 2021, 2:21 PM
  • Add a test for regex_token_iterator.

I will be committing when the CI passes.

zoecarver updated this revision to Diff 338675.Apr 19 2021, 5:11 PM
  • Remove test for iterator_traits<void*> pre-C++17. void can't be referenced so this was failing with a hard error.
ldionne commandeered this revision.Apr 20 2021, 5:59 AM
ldionne edited reviewers, added: zoecarver; removed: ldionne.

Commandeering to fix CI.

ldionne updated this revision to Diff 338845.Apr 20 2021, 6:15 AM

Try to fix CI.

This revision was landed with ongoing or failed builds.Apr 20 2021, 8:30 AM
This revision was automatically updated to reflect the committed changes.