[libcxx-commits] [PATCH] D99461: [libcxx] adds `std::indirectly_readable_traits` to <iterator>

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

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



================
Comment at: libcxx/include/iterator:489
+
+template<class> struct __cond_value_type {};
+
----------------
zoecarver wrote:
> 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.
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
```


================
Comment at: libcxx/include/iterator:525
+
+// TODO(cjdb): add iter_value_t once iterator_traits is cleaned up.
 #endif // !defined(_LIBCPP_HAS_NO_RANGES)
----------------
zoecarver wrote:
> Let's track this TODO once D100393 lands.
It has landed now, let's track it there instead.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp:114
+
+static_assert(
+    check_explicit_member<possibly_different_cv_qualifiers<int, int>, int>());
----------------
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99461/new/

https://reviews.llvm.org/D99461