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

[Christopher Di Bella via Phabricator via libcxx-commits]

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

Please also add tests to ensure that all standard library iterators are compatible with `iterator_traits`. It would be a shame if we introduced a regression.



================
Comment at: libcxx/include/iterator:702-707
+  __has_member_reference<_Ip> &&
+  __has_member_iterator_category<_Ip> &&
+  requires {
+    typename _Ip::difference_type;
+    typename _Ip::value_type;
+  };
----------------
I know I wrote this, but it'd be better if we used nested requires expressions so the requirements can be congruently ordered with the wording.


================
Comment at: libcxx/include/iterator:719-722
+// [iterator.traits]/3.4
+// Otherwise, `iterator_traits<I>` has no members by any of the above names.
+template<class>
+struct __iterator_traits {};
----------------
I'd prefer that we forward declare `__iterator_traits` and then define it under [iterator.traits]/3.3. That's what we did for [[ https://reviews.llvm.org/D96657#inline-913994 | `common_reference` ]], and I think that helped improve readability. Similarly elsewhere.


================
Comment at: libcxx/include/iterator:753
+template<__has_member_pointer _Ip>
+struct __iterator_traits_member_pointer<_Ip> { using type = typename _Ip::pointer; };
+
----------------
To distinguish this from `member_pointer_or_void` we should rename to `member_pointer_or_arrow_or_void`


================
Comment at: libcxx/include/iterator:810
+// [iterator.traits]/3.2.3
+// If the qualified-id `I::iterator­category` is valid and denotes a type, `iterator­category` names
+// that type.
----------------
Typo (scan the comments for other typos such as `iteratortraits` too).


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:27-37
+
+using PointerIteratorTraits = std::iterator_traits<int*>;
+static_assert(std::same_as<PointerIteratorTraits::iterator_category,
+                           std::random_access_iterator_tag>);
+static_assert(std::same_as<PointerIteratorTraits::value_type, int>);
+static_assert(
+    std::same_as<PointerIteratorTraits::difference_type, std::ptrdiff_t>);
----------------
`libcxx/test/std/iterators/iterator.primitives/iterator.traits/pointer.pass.cpp` already covers this case.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:39-48
+using ConstPointerIteratorTraits = std::iterator_traits<const int*>;
+static_assert(std::same_as<ConstPointerIteratorTraits::iterator_category,
+                           std::random_access_iterator_tag>);
+static_assert(std::same_as<ConstPointerIteratorTraits::value_type, int>);
+static_assert(
+    std::same_as<ConstPointerIteratorTraits::difference_type, std::ptrdiff_t>);
+static_assert(std::same_as<ConstPointerIteratorTraits::reference, const int&>);
----------------
`libcxx/test/std/iterators/iterator.primitives/iterator.traits/const_pointer.pass.cpp` has 5/6 of these tests. Please move the `iterator_concept` case there.

`libcxx/test/std/iterators/iterator.primitives/iterator.traits/volatile_pointer.pass.cpp` and `libcxx/test/std/iterators/iterator.primitives/iterator.traits/const_volatile_pointer.pass.cpp` will need updates too.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:49
+                           std::contiguous_iterator_tag>);
+
+using VectorIteratorTraits = std::iterator_traits<std::vector<int>::iterator>;
----------------
cjdb wrote:
> I think these should be `std::same_as<VectorIteratorTraits::member, std::vector<int>::iterator::member>`. I don't particularly care about the concrete types they resolve to, but I do care that they match the members of said iterator.
> 
> I also notice there's no test for whether or not `vector` iterators have `iterator_concept`.
It'd be great if you could section off each subset of tests with the paragraph numbers they correspond to.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:49-57
+
+using VectorIteratorTraits = std::iterator_traits<std::vector<int>::iterator>;
+static_assert(std::same_as<VectorIteratorTraits::iterator_category,
+                           std::random_access_iterator_tag>);
+static_assert(std::same_as<VectorIteratorTraits::value_type, int>);
+static_assert(
+    std::same_as<VectorIteratorTraits::difference_type, std::ptrdiff_t>);
----------------
I think these should be `std::same_as<VectorIteratorTraits::member, std::vector<int>::iterator::member>`. I don't particularly care about the concrete types they resolve to, but I do care that they match the members of said iterator.

I also notice there's no test for whether or not `vector` iterators have `iterator_concept`.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:108
+  struct iterator_category {};
+  struct iterator_concept {}; // ignored
+  struct value_type {};
----------------
Please indicate why. It took me a moment to realise this is expected.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:126
+
+struct LegacyInput {
+  struct iterator_category {};
----------------
Please also add tests for the minimalistic iterator archetypes in `test/libcxx/iterators/iterator.requirements/iterator.assoc.types/iterator.traits/iterator_traits_cpp17_iterators.h`.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:153
+
+struct LegacyInputNoValueType {
+  struct not_value_type {};
----------------
I think we should assert that each of these types actually meet/don't meet the `cpp17-*-iterator` requirements we expect them to. Also, @ldionne asked that the concepts be renamed to `__iterator_traits_detail::cpp17_*_iterator`, so I think we should reflect that update here.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:244
+
+  // Providing difference_type does not fully compensate for missing operator-(It, It).
+  friend MissingMinusIterIter operator-(MissingMinusIterIter, int);
----------------
I think declaring the operator as deleted would be a good idea.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/empty.fail.cpp:103
     }
-
+#endif
     {
----------------
zoecarver wrote:
> cjdb wrote:
> > zoecarver wrote:
> > > Nit: `// _LIBCPP_STD_VER <= 17 || defined(_LIBCPP_HAS_NO_CONCEPTS)`
> > This should be `!defined(_LIBCPP_HAS_NO_RANGES)` now (I didn't get around to fixing it up).
> Actually, I don't think so. This is in the `std/` directory. If it was in the libc++ directory, I would agree. But ideally these are "generic" standard library tests. They wouldn't rely on any of our implementation details. 
In that case, please switch to feature-test macros.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99855