[libcxx-commits] [PATCH] D99855: [libcxx] makes `iterator_traits` C++20-aware

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added a comment.

The logic LGTM.



================
Comment at: libcxx/include/iterator:735
+template<__specifies_members _Ip>
+struct __iterator_traits<_Ip> {
+  using iterator_category  = typename _Ip::iterator_category;
----------------
Can you define this //before// `iterator_traits`? I feel like there's needless complexity here. 


================
Comment at: libcxx/include/iterator:758
+struct __iterator_traits<_Ip> {
+  using iterator_category = typename __iterator_traits_iterator_category<_Ip>::type;
+  using value_type        = typename indirectly_readable_traits<_Ip>::value_type;
----------------
Again, is there any reason we can't define these //before// we use them?


================
Comment at: libcxx/include/iterator:874
+struct __iterator_traits {};
+#else
 template <class _Iter, bool> struct __iterator_traits {};
----------------
Can you put a comment `// !defined...` here too?


================
Comment at: libcxx/include/iterator:916
+#endif
 struct _LIBCPP_TEMPLATE_VIS iterator_traits<_Tp*>
 {
----------------
This also needs a few updated: `value_type` should now remove cv, adds `iterator_concept `, adds a const specialization. Are you making those changes in another patch?


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:82
+template <class I, class Wrapper>
+struct get_reference {
+  using type = std::iter_reference_t<Wrapper>;
----------------
I feel like both this and `get_pointer` are doing the thing where you write the same implementation twice and test that they do the same thing. IMHO that's not a great way to write tests. 


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:107
+template <class I, class Wrapper, class Category>
+[[nodiscard]] constexpr bool check_wrapper_traits() {
+  using Traits = std::iterator_traits<Wrapper>;
----------------
Side note: the functions don't actually return anything useful. All the testing is done when the function is instantiated. So, why is this nodiscard? Wouldn't instantiating this function template inside a function be the same as instantiating it inside a static_assert?


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:125
+requires(sizeof...(Args) > 0) constexpr
+    void check_with_legacy_input_iterator() {
+  {
----------------
This indentation seems a bit odd, is this what clang format spit out? Either way, can you un-indent? 


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:162
+template <class I>
+[[nodiscard]] constexpr bool check_with_legacy_iterator() {
+  {
----------------
This test I really like because I can read it and immediately see that it's doing the right thing. You say, "here is the input" and "here is the non-computed literal output."


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:251
+
+  inline static constexpr int value_type = 0;
+};
----------------
Nice. This is a good idea to test :)


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:289
+};
+static_assert(check_fails<reference_as_static_member>());
----------------
I have no idea what the consensus here was... are we still doing compile.passp.cpp tests or no? If no, then we should make this a "normal" .pass test.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/empty.fail.cpp:103
     }
-
+#endif
     {
----------------
Nit: `// _LIBCPP_STD_VER <= 17 || defined(_LIBCPP_HAS_NO_CONCEPTS)`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99855