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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added a comment.

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



================
Comment at: libcxx/include/iterator:517
+struct indirectly_readable_traits<_Tp>
+  : __cond_value_type<typename _Tp::value_type> {};
+
----------------
zoecarver wrote:
> cjdb wrote:
> > Quuxplusone wrote:
> > > 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?
> > > 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.
> So there's no functional difference here? This overload just catches the case where `element_type == value_type` is `false` (on GCC)? 
@zoecarver I'm not sure what you're asking about.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp:23
+
+#pragma GCC diagnostic ignored "-Wunused-local-typedefs"
+
----------------
zoecarver wrote:
> What's the unused typedef? 
All the `value_type` and `element_type` aliases. I've added a comment.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp:83
+  constexpr bool result = check_value_type_matches<T, Expected>;
+  return result == check_value_type_matches<T const, Expected>;
+}
----------------
zoecarver wrote:
> 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;`? 
Reverted to the @CaseyCarter method.


================
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>());
----------------
ldionne wrote:
> cjdb wrote:
> > ldionne wrote:
> > > 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?
> > Yes, I can apply that to D99691 and then let clang-format take over. D99691 needs an LGTM though.
> 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 :-)
120 columns seems to have done the trick (fun fact: the last line is 119 characters long 😉).


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp:186
+
+  return result;
+}
----------------
zoecarver wrote:
> What about `&& result == false;` because this should never be true AFAICT. 
I'm a bit confused. `result && result == false` is a contradiction?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99461