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.
Details
Details
- Reviewers
ldionne EricWF Mordante curdeius • Quuxplusone tcbrindle zoecarver miscco - Group Reviewers
Restricted Project - Commits
- rGf280505aa084: [libcxx] adds `std::indirectly_readable_traits` to <iterator>
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| libcxx/include/iterator | ||
|---|---|---|
| 481 | Indent line 477 one level (two spaces). Ditto line 488. | |
| 517 | 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>); | |
| 521 | 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 | ||
| 28 ↗ | (On Diff #333705) | > && |
| 66 ↗ | (On Diff #333705) | This function should use regular assert and return true; at the end. |
| 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. |
| libcxx/include/iterator | ||
|---|---|---|
| 481 | It's not misformatted, so there's nothing to fix. | |
| 521 |
Done.
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) |
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.
Done. |
| 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.) |
Comment Actions
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. |
| 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. | |
| 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. | |
| 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.) | |
| 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. | |
Comment Actions
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? | |
Comment Actions
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? | |
| 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. | |
Comment Actions
I removed the dependency on D99141 because it already landed. This was making the phab CLI confused.
| 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? | |
| libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp | ||
|---|---|---|
| 23 | Why is that necessary? | |
| 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. | |
| 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. | |
| 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. | |
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.)