[libcxx] adds `std::indirectly_readable_traits` to <iterator>

[cjdb]

Summary

Implements parts of:

• P0896R4 The One Ranges Proposal
• LWG3446 indirectly_readable_traits ambiguity for types with both value_type and element_type

Depends on D99141.

[cjdb]

adds test coverage and marks LWG3446 as complete

[cjdb]

libcxx/include/iterator

477

It's not misformatted, so there's nothing to fix.

517

there's probably some .csv file that needs to be updated to say that we implement the fix

Done.

Er, actually, line 517 should say

: __cond_value_type<remove_cv_t<typename _Tp::value_type>> {};

according to https://en.cppreference.com/w/cpp/iterator/indirectly_readable_traits , which implies that LWG3446 isn't telling the whole story. What's the deal here?

struct S {
  using value_type = int const volatile;
  using element_type = int const volatile;
};

It's left as an exercise for the reader to evaluate the type std::indirectly_readable_traits<S>::type by hand.

libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.pass.cpp

67

See https://reviews.llvm.org/D99141#inline-935428.

108

AIUI, the point of indirectly_readable is that std::vector<int>::iterator should pass this test, and it's just an accident that std::vector and std::optional themselves pass it.

I was under the impression that they weren't supported by indirectly_readable_traits, but it so happens that they are, and they're only blocked by indirectly_readable. Since I found that surprising, I decided that including them as tests would serve as good documentation.

Check vector<int>::{const_,}iterator.
If we're checking specific library types, I'd probably add istream_iterator<int>.

Done.

[zoecarver]

libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.pass.cpp

67

I'm not sure what we get from using regular assert and returning true. We made this the standard quo for updating algorithms, etc. to be constexpr, because we wanted to preserve the existing test coverage in a constexpr context (i.e., both at runtime and during constexpr evaluation the same tests would be run), but that doesn't really apply here. These tests are only run in a constexpr context.

In fact, I think it would actually be worse to use regular assert because that will give a confusing error and likely be much slower. (Regular assert will result in a "is not an integral constant" error whereas the static_assert will give the full stack trace, I think.)

[cjdb]

rebases to activate CI

[cjdb]

rebases to activate CI

[cjdb]

adds pragma to test to silence GCC warning

libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.pass.cpp

67

Even if it doesn't give a full stack trace, "is false" is much clearer than "is not an integral constant", since the latter could be issued for a number of reasons.

[cjdb]

fixes GCC compile-time error with a workaround

[tcanens]

libcxx/include/iterator

515

What does "placate GCC" mean in this context? The standard implies that this case (have both, but inconsistent) is a hard error, since we can't decide on the specialization to pick. It's certainly debatable whether it should be though.

[cjdb]

libcxx/include/iterator

515

Both GCC trunk and MSVC (with their respective native libraries) compile the test file (and IIRC so does MSVC with this implementation regardless of the specialisation we're discussing). I happen to agree with the implementations being the right course of action, even if the wording implies something else.

Maybe I should issue a library defect if one isn't already in the pipeline.

[tcanens]

libcxx/include/iterator

515

Fascinating. Looks like both MSVC and Clang treat the ambiguous partial specialization as a concept check failure rather than a hard error - but MSVC doesn't SFINAE on it... (libstdc++ adds this same specialization.)

[cjdb]

rebases to activate CI

[zoecarver]

libcxx/include/iterator

485

Nit: can you put __cond_value_type and __has_member_value_type at the top? Then we can read all the indirectly_readable_traits overloads in a row.

517

So there's no functional difference here? This overload just catches the case where element_type == value_type is false (on GCC)?

521

Let's track this TODO once D100393 lands.

libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp

23 ↗

(On Diff #336693)

What's the unused typedef?

83 ↗

(On Diff #336693)

If both check_value_type_matches<T, ... and check_value_type_matches<T const, ...were false, check_explicit_member would return true, right? Can you make this return ... && result == true;?

107 ↗

(On Diff #336693)

Is clang-format off not working or did you actually write out > > :P

(Just joking nothing do fix here.)

186 ↗

(On Diff #336693)

What about && result == false; because this should never be true AFAICT.

[zoecarver]

This LGTM, though. Other than the small comments.

[ldionne]

Contents of the patch LGTM, some comments on readability. This LGTM once addressed.

libcxx/include/iterator

485

Yup, I agree, I would actually organize it this way to increase readability:

template<class> struct __cond_value_type {};
template<class _Tp>
requires is_object_v<_Tp>
struct __cond_value_type<_Tp> { using value_type = remove_cv_t<_Tp>; };

template<class _Tp>
concept __has_member_value_type = requires { typename _Tp::value_type; };

template<class _Tp>
concept __has_member_element_type = requires { typename _Tp::element_type; };

// Then all the indirectly_readable_traits implementation

521

It has landed now, let's track it there instead.

libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp

114 ↗

(On Diff #336693)

I know we don't want to spend too much time on formatting, however as I've said elsewhere, readability is still a major goal. I don't know if you'd agree, but this is very difficult to read IMO. The fact that we both use long identifier names and a not-super-long line wrap setting means that we get line breaks at random places. Aligning stuff would make the test easier to read, do you agree?

static_assert(check_explicit_member<possibly_different_cv_qualifiers<int, int>, int>());
static_assert(check_explicit_member<possibly_different_cv_qualifiers<int, int const>, int>());
static_assert(check_explicit_member<possibly_different_cv_qualifiers<int, int volatile>, int>());
static_assert(check_explicit_member<possibly_different_cv_qualifiers<int, int const volatile>, int>());
static_assert(check_explicit_member<possibly_different_cv_qualifiers<int const, int>, int>());
static_assert(check_explicit_member<possibly_different_cv_qualifiers<int const, int const>, int>());
static_assert(check_explicit_member<possibly_different_cv_qualifiers<int const, int volatile>, int>());
static_assert(check_explicit_member<possibly_different_cv_qualifiers<int const, int const volatile>, int>());
static_assert(check_explicit_member<possibly_different_cv_qualifiers<int volatile, int>, int>());
static_assert(check_explicit_member<possibly_different_cv_qualifiers<int volatile, int const>, int>());
static_assert(check_explicit_member<possibly_different_cv_qualifiers<int volatile, int volatile>, int>());
static_assert(check_explicit_member<possibly_different_cv_qualifiers<int volatile, int const volatile>, int>());
static_assert(check_explicit_member<possibly_different_cv_qualifiers<int const volatile, int>, int>());
static_assert(check_explicit_member<possibly_different_cv_qualifiers<int const volatile, int const>, int>());
static_assert(check_explicit_member<possibly_different_cv_qualifiers<int const volatile, int volatile>, int>());
static_assert(check_explicit_member<possibly_different_cv_qualifiers<int const volatile, int const volatile>, int>());

Do you think there's a way to tweak the Clang format configuration to make this happen automatically?

[cjdb]

libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp

114 ↗

(On Diff #336693)

Yes, I can apply that to D99691 and then let clang-format take over. D99691 needs an LGTM though.

[ldionne]

libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp

114 ↗

(On Diff #336693)

Feel free to tweak/remove the column limit in D99691 or in a separate patch. The only thing that's important for me is that I can read the code easily :-)

[cjdb]

applies feedback issued by @zoecarver and @ldionne

[cjdb]

Will merge after @ldionne green tick and @zoecarver clarifies a few points.

libcxx/include/iterator

517

@zoecarver I'm not sure what you're asking about.

libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp

23 ↗

(On Diff #336693)

All the value_type and element_type aliases. I've added a comment.

83 ↗

(On Diff #336693)

Reverted to the @CaseyCarter method.

114 ↗

(On Diff #336693)

120 columns seems to have done the trick (fun fact: the last line is 119 characters long 😉).

186 ↗

(On Diff #336693)

I'm a bit confused. result && result == false is a contradiction?

[cjdb]

applies feedback

[cjdb]

rebase went wrong, reverting what should've been applied to D99854

[zoecarver]

libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp

186 ↗

(On Diff #336693)

It's a moot point now that you're returning result, but I was saying, you could verify that check_has_value_type<ref_element> == result && result == false.

[zoecarver]

I removed the dependency on D99141 because it already landed. This was making the phab CLI confused.

[ldionne]

🛳 it!

[cjdb]

rebases to activate CI

[cjdb]

rebases to activate CI

[zoecarver]

libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp

22 ↗

(On Diff #337871)

Forgot one thing: can you please include test_macros.h here and in all your other patches/test files?

[cjdb]

rebases to activate CI (and gets it right this time)

[cjdb]

libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp

22 ↗

(On Diff #337871)

Why is that necessary?

[zoecarver]

libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp

22 ↗

(On Diff #337871)

I thought we had a discussion in discord that we were always going to include this in all passing test files. That way we can easily grep and verify that every file has it. (I made a commit a while back to update all files that didn't include it.)

The main reason it's necessary is to prevent people from accidentally using TEST_STD_VER when it's not defined. There have been several instances where that has disabled test code without any warning/error.

[curdeius]

libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp

22 ↗

(On Diff #337871)

I've recently committed addition of -Wundef that should prevent this sort of errors so including this header everywhere is not really necessary IMO.

[zoecarver]

libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp

22 ↗

(On Diff #337871)

Okay, in that case I don't feel strongly as to whether you add it or not.