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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a subscriber: zoecarver.
ldionne added inline comments.


================
Comment at: libcxx/include/__memory/raw_storage_iterator.h:45
+    typedef void                pointer;
+    typedef void                reference;
+
----------------
Mordante wrote:
> Is it intended to change the types of these typedefs and the base class?
Yes, I'm making it standards conforming. I'll add a test too, it seems like we didn't have any.


================
Comment at: libcxx/include/iterator:508
 
 template<class _Category, class _Tp, class _Distance = ptrdiff_t,
          class _Pointer = _Tp*, class _Reference = _Tp&>
----------------
Quuxplusone wrote:
> 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`.
That's all correct.

However, going forward, I'll be keen to remove things without guarding them with something like `_LIBCPP_ENABLE_CXX23_REMOVED_ITERATOR`: if you opt-in to a newer standard mode, it means you're OK with the removal. So, for example, I'd be fine if we had not guarded the removal of `std::raw_storage_iterator` by `_LIBCPP_ENABLE_CXX20_REMOVED_RAW_STORAGE_ITERATOR`, but it's fine too cause it's done now.


================
Comment at: libcxx/include/iterator:816
+    typedef void value_type;
+    typedef void difference_type;
+    typedef void pointer;
----------------
Quuxplusone wrote:
> 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).
I was planning on a different review where I update all the iterator types like you did in D103101 (and @zoecarver in his own similar patch).


================
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*;
----------------
Quuxplusone wrote:
> 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.
I think it's safe to assume there was no intent to test `std::iterator` specifically here. Agreed about the drive-by change.


================
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*;
----------------
Quuxplusone wrote:
> 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.
Yup, I agree this is more straightforward.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103171