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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

I really like the mixing to the Standard's wording with the code, makes it a lot easier to follow and review!



================
Comment at: libcxx/include/iterator:712
 
+template<class> struct __iterator_traits_member_pointer;
+template<class> struct __iterator_traits_member_reference;
----------------
Does this hunk affect the synopsis?


================
Comment at: libcxx/include/iterator:732
+// [iterator.traits]/3.1
+// If I has valid ([temp.deduct]) member types `difference_­type`, `value_­type`, `reference`, and
+// `iterator_­category`, then `iterator_­traits<I>` has the following publicly accessible members:
----------------
For clarity "If I has" -> "If `I` has".


================
Comment at: libcxx/include/iterator:741
+  using reference          = typename _Ip::reference;
+  using __primary_template = iterator_traits;
+};
----------------
Will `__primary_template` be used in a later patch?


================
Comment at: libcxx/include/iterator:755
+// [iterator.traits]/3.2
+// Otherwise, if `I` satisfies the exposition-only concept _`cpp17-input-iterator`_,
+// `iterator_­traits<I>` has the following publicly accessible members:
----------------
I rather remove the underscores in _`cpp17-input-iterator`_,". I don't think they add clarity. The same for the other places where you used these concepts.


================
Comment at: libcxx/include/iterator:908
+#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_CONCEPTS)
+requires is_object_v<_Tp>
+#endif
----------------
Please update the synopsis.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.pass.cpp:16
+
+#include <iterator>
+
----------------
I would like some tests using the `contiguous_iterator_tag` see whether it acts as a `random_access_iterator_tag`.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.pass.cpp:183
+    static_assert(std::same_as<typename iterator_traits::difference_type,
+                               typename iterator_traits::difference_type>);
+    static_assert(std::same_as<typename iterator_traits::reference, void>);
----------------
`std::same_as` compares the same type.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.pass.cpp:226
+  using reference = int&;
+};
+static_assert(check_fails<missing_iterator_category>());
----------------
Would it be nice to test whether the test fails when missing_iterator_category is provided but not a typename? E.g. 
`int missing_iterator_category`
And similar for the tests below?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99855