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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/include/iterator:517
+struct indirectly_readable_traits<_Tp>
+  : __cond_value_type<typename _Tp::value_type> {};
+
----------------
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)? 


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


================
Comment at: libcxx/include/iterator:525
+
+// TODO(cjdb): add iter_value_t once iterator_traits is cleaned up.
 #endif // !defined(_LIBCPP_HAS_NO_RANGES)
----------------
Let's track this TODO once D100393 lands.


================
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"
+
----------------
What's the unused typedef? 


================
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>;
+}
----------------
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;`? 


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp:107
+template<class T, class U>
+requires std::same_as<std::remove_cv_t<T>, std::remove_cv_t<U> >
+struct possibly_different_cv_qualifiers {
----------------
Is clang-format off not working or did you actually write out `> >` :P 

(Just joking nothing do fix here.)


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/readable.traits/indirectly_readable_traits.compile.pass.cpp:186
+
+  return result;
+}
----------------
What about `&& result == false;` because this should never be true AFAICT. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99461