[libcxx-commits] [PATCH] D103171: [libc++] Deprecate std::iterator and remove it as a base class

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

LGTM % comments!



================
Comment at: libcxx/include/iterator:508
 
 template<class _Category, class _Tp, class _Distance = ptrdiff_t,
          class _Pointer = _Tp*, class _Reference = _Tp&>
----------------
Mordante wrote:
> Can we remove this class when `_LIBCPP_ABI_NO_ITERATOR_BASES` is defined?
No. The //existence// of this class doesn't have ABI implications. Also, this class is still present (but deprecated) in C++20. https://timsong-cpp.github.io/cppwp/n4861/depr.iterator.basic#lib:iterator
If this class were removed from C++23, we'd want to guard it under an //API// macro such as `_LIBCPP_ENABLE_CXX23_REMOVED_ITERATOR`.


================
Comment at: libcxx/include/iterator:816
+    typedef void value_type;
+    typedef void difference_type;
+    typedef void pointer;
----------------
D103101 updates this code for C++20 (where `difference_type` becomes `ptrdiff_t` instead of `void`). Do you want to roll that in here á là D103101, or would you like a followup PR for that?
Notice that we want to change the value of the //typedef// in C++20, but not alter the name of the //base class// (because that would break ABI).


================
Comment at: libcxx/test/libcxx/iterators/iterator.requirements/iterator.concepts/cpp20_iter_concepts.pass.cpp:49-63
 namespace std {
 template <>
-struct iterator_traits<MyIter3>
-    : std::iterator<OtherTagTwo, char> {};
+struct iterator_traits<MyIter3> {
+  using iterator_category = OtherTagTwo;
+  using value_type = char;
+  using difference_type = std::ptrdiff_t;
+  using pointer = char*;
----------------
These changes look OK to me, but it would be useful to know what the author was originally trying to test here: this test didn't have anything to do with `std::iterator`'s behavior, did it?
I suggest drive-by changing
```
namespace std {
template <>
struct iterator_traits<MyIter3> {
```
to
```
template <>
struct std::iterator_traits<MyIter3> {
```
both to save LOC and to avoid reopening namespace std.


================
Comment at: libcxx/test/libcxx/iterators/iterator.requirements/iterator.concepts/cpp20_iter_traits.pass.cpp:45-64
 namespace std {
 template <>
-struct iterator_traits<MyIter>
-    : iterator_traits<std::iterator<std::random_access_iterator_tag, char> > {};
+struct iterator_traits<MyIter> {
+  using iterator_category = std::random_access_iterator_tag;
+  using value_type = char;
+  using difference_type = std::ptrdiff_t;
+  using pointer = char*;
----------------
Same comment here about reopening namespace std.
But also here it is //clear// that `MyIter` and `MyIter2` are exactly equivalent, and so you can eliminate one of them.
It looks like the point of this test is to verify that `std::_ITER_TRAITS<T>` is `std::iterator_traits<T>` when `T` specializes `iterator_traits`, and `T` when it doesn't. So the entire test file should look like
```
#include <iterator>
#include "test_iterators.h"
struct A : random_access_iterator<int*> {};
struct B : random_access_iterator<int*> {};
struct C : random_access_iterator<int*> {};
struct D : random_access_iterator<int*> {};
template<> struct std::iterator_traits<B> {};
template<> struct std::iterator_traits<C> : std::iterator_traits<A> {};
template<> struct std::iterator_traits<D> : std::iterator_traits<int*> {};

static_assert(std::is_same_v<std::_ITER_TRAITS<int*>, std::iterator_traits<int*>>);
static_assert(std::is_same_v<std::_ITER_TRAITS<A>, A>);
static_assert(std::is_same_v<std::_ITER_TRAITS<B>, std::iterator_traits<B>>);
static_assert(std::is_same_v<std::_ITER_TRAITS<C>, std::iterator_traits<C>>);
static_assert(std::is_same_v<std::_ITER_TRAITS<D>, std::iterator_traits<D>>);
```
and it should be a .compile.pass.cpp test.


================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iterator/types.pass.cpp:56
+#if TEST_STD_VER <= 14
+    typedef std::iterator<typename T::iterator_category, char> iterator_base;
+    static_assert((std::is_base_of<iterator_base, R>::value), "");
----------------
Probably should say `typename R::value_type` instead of `char`, for consistency with lines 49-53.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103171