Page MenuHomePhabricator

[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

There are a very large number of changes, so older changes are hidden. Show Older Changes
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
481

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

521

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
66 ↗(On Diff #333705)
107 ↗(On Diff #333705)

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
66 ↗(On Diff #333705)

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
66 ↗(On Diff #333705)

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
519

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
519

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
519

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.Tue, Apr 13, 11:21 AM
libcxx/include/iterator
489

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.

521

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

525

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
24

What's the unused typedef?

84

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

108

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

(Just joking nothing do fix here.)

187

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

zoecarver accepted this revision as: zoecarver.Tue, Apr 13, 11:22 AM

This LGTM, though. Other than the small comments.

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

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

libcxx/include/iterator
489

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
525

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
115

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.Wed, Apr 14, 9:13 AM
cjdb added inline comments.Wed, Apr 14, 11:31 AM
libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp
115

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

ldionne added inline comments.Wed, Apr 14, 1:58 PM
libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp
115

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.Wed, Apr 14, 8:09 PM
cjdb marked 10 inline comments as done.

applies feedback issued by @zoecarver and @ldionne

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

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

libcxx/include/iterator
521

@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
24

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

84

Reverted to the @CaseyCarter method.

115

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

187

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

cjdb updated this revision to Diff 337618.Wed, Apr 14, 8:38 PM

applies feedback

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

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

zoecarver added inline comments.Wed, Apr 14, 9:33 PM
libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp
187

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.Thu, Apr 15, 9:34 AM

🛳 it!

This revision is now accepted and ready to land.Thu, Apr 15, 9:34 AM
zoecarver added inline comments.Thu, Apr 15, 12:33 PM
libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp
23

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.Thu, Apr 15, 12:33 PM

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

cjdb added inline comments.Thu, Apr 15, 12:36 PM
libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp
23

Why is that necessary?

zoecarver added inline comments.Thu, Apr 15, 12:59 PM
libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp
23

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.Thu, Apr 15, 1:04 PM
libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp
23

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.Thu, Apr 15, 1:11 PM
libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp
23

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