This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by cjdb on Mar 27 2021, 7:55 PM.

Details

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.

Diff Detail

Event Timeline

cjdb requested review of this revision.Mar 27 2021, 7:55 PM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2021, 7:55 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
libcxx/include/iterator
477

Indent line 477 one level (two spaces). Ditto line 488.
(Please no objections along the lines of "but clang-format did it this way." We shouldn't check in misformatted code just because a machine produced it.)

513

Make sure you have a test that fails if either of these remove_cv_t are missing. I suggest

struct X1 { using value_type = int; using element_type = const volatile int; };
struct X2 { using value_type = const volatile int; using element_type = int; };
static_assert(std::is_same_v<indirectly_readable_traits<X1>::value_type, int>);
static_assert(std::is_same_v<indirectly_readable_traits<X2>::value_type, int>);
517

This implements https://cplusplus.github.io/LWG/issue3446 — excellent! — but that means there's probably some .csv file that needs to be updated to say that we implement the fix.

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?

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

> &&

67

This function should use regular assert and return true; at the end.

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.
Check vector<int>::{const_,}iterator. Checking vector and optional seems fine but also unnecessary.
If we're checking specific library types, I'd probably add istream_iterator<int>.

cjdb updated this revision to Diff 333774.Mar 28 2021, 11:48 PM
cjdb marked 4 inline comments as done.Mar 28 2021, 11:48 PM
cjdb edited the summary of this revision. (Show Details)

adds test coverage and marks LWG3446 as complete

cjdb added inline comments.Mar 28 2021, 11:51 PM
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
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 added inline comments.Mar 29 2021, 11:20 PM
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 updated this revision to Diff 335058.Apr 2 2021, 9:35 PM

rebases to activate CI

cjdb updated this revision to Diff 335202.Apr 4 2021, 10:56 PM

rebases to activate CI

cjdb updated this revision to Diff 335412.Apr 5 2021, 11:23 PM
cjdb marked an inline comment as done.

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 updated this revision to Diff 335716.Apr 6 2021, 9:35 PM

fixes GCC compile-time error with a workaround

tcanens added a subscriber: tcanens.Apr 7 2021, 7:19 PM
tcanens added inline comments.
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 added inline comments.Apr 7 2021, 7:43 PM
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 added inline comments.Apr 7 2021, 7:57 PM
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 updated this revision to Diff 336693.Apr 11 2021, 2:35 PM

rebases to activate CI

zoecarver added inline comments.Apr 13 2021, 11:21 AM
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 accepted this revision as: zoecarver.Apr 13 2021, 11:22 AM

This LGTM, though. Other than the small comments.

ldionne requested changes to this revision.Apr 14 2021, 9:13 AM

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?

This revision now requires changes to proceed.Apr 14 2021, 9:13 AM
cjdb added inline comments.Apr 14 2021, 11:31 AM
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 added inline comments.Apr 14 2021, 1:58 PM
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 updated this revision to Diff 337617.Apr 14 2021, 8:09 PM
cjdb marked 10 inline comments as done.

applies feedback issued by @zoecarver and @ldionne

cjdb added a comment.Apr 14 2021, 8:09 PM

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

applies feedback

cjdb updated this revision to Diff 337620.Apr 14 2021, 8:47 PM

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

zoecarver added inline comments.Apr 14 2021, 9:33 PM
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.

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

ldionne accepted this revision.Apr 15 2021, 9:34 AM

🛳 it!

This revision is now accepted and ready to land.Apr 15 2021, 9:34 AM
zoecarver added inline comments.Apr 15 2021, 12:33 PM
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 updated this revision to Diff 337872.Apr 15 2021, 12:33 PM

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

cjdb added inline comments.Apr 15 2021, 12:36 PM
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 added inline comments.Apr 15 2021, 12:59 PM
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 added inline comments.Apr 15 2021, 1:04 PM
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 added inline comments.Apr 15 2021, 1:11 PM
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.