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

[• Quuxplusone]

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]

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.

libcxx/docs/Cxx2aStatusIssuesStatus.csv

 "`3388 <https://wg21.link/LWG3388>`__","``view``\  iterator types have ill-formed ``<=>``\  operators","Prague","",""
 "`3389 <https://wg21.link/LWG3389>`__","A move-only iterator still does not have a ``counted_iterator``\ ","Prague","",""
 "`3390 <https://wg21.link/LWG3390>`__","``make_move_iterator()``\  cannot be used to construct a ``move_iterator``\  for a move-only iterator","Prague","",""
 "`3393 <https://wg21.link/LWG3393>`__","Missing/incorrect feature test macro for coroutines","Prague","",""
 "`3395 <https://wg21.link/LWG3395>`__","Definition for three-way comparison needs to be updated (US 152)","Prague","",""
 "`3396 <https://wg21.link/LWG3396>`__","Clarify point of reference for ``source_location::current()``\  (DE 169)","Prague","",""
 "`3397 <https://wg21.link/LWG3397>`__","``ranges::basic_istream_view::iterator``\  should not provide ``iterator_category``\ ","Prague","",""
 "`3398 <https://wg21.link/LWG3398>`__","``tuple_element_t``\  is also wrong for ``const subrange``\ ","Prague","",""
+"`3446 <https://wg21.link/LWG3446>`__","``indirectly_readable_traits``\ ambiguity for types with both ``value_type``\ and ``element_type``\ ","November virtual meeting","|Complete|","14.0"

libcxx/include/iterator

 #ifndef _LIBCPP_ITERATOR
 #define _LIBCPP_ITERATOR
 
 /*
     iterator synopsis
 
 namespace std
 {
-template<class> struct incrementable_traits; // since C++20
+template<class> struct incrementable_traits;       // since C++20
+template<class> struct indirectly_readable_traits; // since C++20
 
 template<class Iterator>
 struct iterator_traits
 {
     typedef typename Iterator::difference_type difference_type;
     typedef typename Iterator::value_type value_type;
     typedef typename Iterator::pointer pointer;
     typedef typename Iterator::reference reference;

 };
 
 template<__has_integral_minus _Tp>
 struct incrementable_traits<_Tp> {
   using difference_type = make_signed_t<decltype(std::declval<_Tp>() - std::declval<_Tp>())>;
 };
 
 // TODO(cjdb): add iter_difference_t once iterator_traits is cleaned up.
+
+// [readable.traits]
+template<class> struct indirectly_readable_traits {};
+
+template<class _Ip>
+requires is_array_v<_Ip>
+struct indirectly_readable_traits<_Ip> {
+  using value_type = remove_cv_t<remove_extent_t<_Ip>>;
+};
+
+template<class _Ip>
+struct indirectly_readable_traits<const _Ip> : indirectly_readable_traits<_Ip> {};
+
+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>
+struct indirectly_readable_traits<_Tp*> : __cond_value_type<_Tp> {};
+
+template<class _Tp>
+concept __has_member_value_type = requires { typename _Tp::value_type; };
+
+template<__has_member_value_type _Tp>
+struct indirectly_readable_traits<_Tp>
+  : __cond_value_type<typename _Tp::value_type> {};
+
+template<class _Tp>
+concept __has_member_element_type = requires { typename _Tp::element_type; };
+
+template<__has_member_element_type _Tp>
+struct indirectly_readable_traits<_Tp>
+  : __cond_value_type<typename _Tp::element_type> {};
+
+template<class _Tp>
+concept __has_members_value_type_and_element_type =
+  __has_member_value_type<_Tp> &&
+  __has_member_element_type<_Tp> &&
+  same_as<remove_cv_t<typename _Tp::element_type>,
+          remove_cv_t<typename _Tp::value_type>>;
+
+template<__has_members_value_type_and_element_type _Tp>
+struct indirectly_readable_traits<_Tp>
+  : __cond_value_type<typename _Tp::value_type> {};
+
+// TODO(cjdb): add iter_value_t once iterator_traits is cleaned up.
 #endif // _LIBCPP_STD_VER > 17
 
 template <class _Iter>
 struct _LIBCPP_TEMPLATE_VIS iterator_traits;
 
 struct _LIBCPP_TEMPLATE_VIS input_iterator_tag {};
 struct _LIBCPP_TEMPLATE_VIS output_iterator_tag {};
 struct _LIBCPP_TEMPLATE_VIS forward_iterator_tag       : public input_iterator_tag {};
 struct _LIBCPP_TEMPLATE_VIS bidirectional_iterator_tag : public forward_iterator_tag {};
 struct _LIBCPP_TEMPLATE_VIS random_access_iterator_tag : public bidirectional_iterator_tag {};

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

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// UNSUPPORTED: libcpp-no-concepts
+
+// template<class T>
+// struct indirectly_readable_traits;
+
+#include <iterator>
+
+#include <concepts>
+#include <memory>
+#include <optional>
+#include <string>
+#include <vector>
+
+// clang-format off
+template <class T>
+concept check_has_value_type = requires {
+  typename std::indirectly_readable_traits<T>::value_type;
+};
+
+template <class T, class Expected>
+concept check_value_type_matches =
+  check_has_value_type<T> &&
+  std::same_as<typename std::indirectly_readable_traits<T>::value_type, Expected>;
+// clang-format on
+
+template <class T>
+constexpr bool check_pointer() {
+  constexpr bool result = check_value_type_matches<T*, T>;
+  static_assert(check_value_type_matches<T const*, T> == result);
+  static_assert(check_value_type_matches<T volatile*, T> == result);
+  static_assert(check_value_type_matches<T const volatile*, T> == result);
+
+  static_assert(check_value_type_matches<T* const, T> == result);
+  static_assert(check_value_type_matches<T const* const, T> == result);
+  static_assert(check_value_type_matches<T volatile* const, T> == result);
+  static_assert(check_value_type_matches<T const volatile* const, T> == result);
+
+  return result;
+}
+
+static_assert(!check_pointer<void>());
+static_assert(check_pointer<int>());
+static_assert(check_pointer<long>());
+static_assert(check_pointer<double>());
+static_assert(check_pointer<double*>());
+
+struct S {};
+static_assert(check_pointer<S>());
+
+template <class T>
+constexpr bool check_array() {
+  constexpr bool result = check_value_type_matches<T[], T>;
+  static_assert(check_value_type_matches<T const[], T> == result);
+  static_assert(check_value_type_matches<T volatile[], T> == result);
+  static_assert(check_value_type_matches<T const volatile[], T> == result);
+  static_assert(check_value_type_matches<T[10], T> == result);
+  static_assert(check_value_type_matches<T const[10], T> == result);
+  static_assert(check_value_type_matches<T volatile[10], T> == result);
+  static_assert(check_value_type_matches<T const volatile[10], T> == result);
+  return result;
+}
+
+static_assert(check_array<int>());
+static_assert(check_array<long>());
+static_assert(check_array<double>());
+static_assert(check_array<double*>());
+static_assert(check_array<S>());
+
+template <class T, class Expected>
+constexpr bool check_explicit_member() {
+  constexpr bool result = check_value_type_matches<T, Expected>;
+  return result == check_value_type_matches<T const, Expected>;
+}
+
+struct has_value_type {
+  using value_type = int;
+};
+static_assert(check_explicit_member<has_value_type, int>());
+static_assert(check_explicit_member<std::vector<int>::iterator, int>());
+
+struct has_element_type {
+  using element_type = S;
+};
+static_assert(check_explicit_member<has_element_type, S>());
+
+struct has_same_value_and_element_type {
+  using value_type = int;
+  using element_type = int;
+};
+static_assert(check_explicit_member<has_same_value_and_element_type, int>());
+static_assert(check_explicit_member<std::shared_ptr<long>, long>());
+static_assert(check_explicit_member<std::shared_ptr<long const>, long>());
+
+// clang-format off
+template<class T, class U>
+requires std::same_as<std::remove_cv_t<T>, std::remove_cv_t<U> >
+struct possibly_different_cv_qualifiers {
+  using value_type = T;
+  using element_type = U;
+};
+// clang-format on
+
+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>());
+
+struct S2 {};
+namespace std {
+template <>
+struct indirectly_readable_traits<S2> {
+  using value_type = int;
+};
+} // namespace std
+static_assert(check_value_type_matches<S2, int>);
+static_assert(check_value_type_matches<std::vector<int>, int>);
+static_assert(check_value_type_matches<std::vector<int>::iterator, int>);
+static_assert(check_value_type_matches<std::vector<int>::const_iterator, int>);
+static_assert(check_value_type_matches<std::istream_iterator<int>, int>);
+static_assert(check_value_type_matches<std::istreambuf_iterator<char>, char>);
+static_assert(check_value_type_matches<std::optional<int>, int>);
+
+template <class T>
+constexpr bool check_ref() {
+  struct ref_value {
+    using value_type = T&;
+  };
+  constexpr bool result = check_has_value_type<ref_value>;
+
+  struct ref_element {
+    using element_type = T&;
+  };
+  static_assert(check_has_value_type<ref_element> == result);
+
+  return result;
+}
+
+static_assert(!check_ref<int>());
+static_assert(!check_ref<S>());
+static_assert(!check_ref<std::shared_ptr<long> >());
+
+static_assert(!check_has_value_type<void>);
+static_assert(!check_has_value_type<std::nullptr_t>);
+static_assert(!check_has_value_type<int>);
+static_assert(!check_has_value_type<S>);
+
+struct has_different_value_and_element_type {
+  using value_type = int;
+  using element_type = long;
+};
+static_assert(!check_has_value_type<has_different_value_and_element_type>);
+
+struct void_value {
+  using value_type = void;
+};
+static_assert(!check_has_value_type<void_value>);
+
+struct void_element {
+  using element_type = void;
+};
+static_assert(!check_has_value_type<void_element>);
+
+int main(int, char**) { return 0; }