[libcxx-commits] [PATCH] D101729: [libcxx] deprecates `std::iterator` and removes it as a base class

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

Quuxplusone added a comment.

Looks pretty good to me, mod comments.



================
Comment at: libcxx/include/__memory/raw_storage_iterator.h:30-36
+#if _LIBCPP_STD_VER <= 14
     : public iterator<output_iterator_tag,
                       _Tp,                                         // purposefully not C++03
                       ptrdiff_t,                                   // purposefully not C++03
                       _Tp*,                                        // purposefully not C++03
                       raw_storage_iterator<_OutputIterator, _Tp>&> // purposefully not C++03
+#endif
----------------
Interestingly, libc++ has never been conforming here.
https://timsong-cpp.github.io/cppwp/n3337/storage.iterator
requires inheriting from `iterator<output_iterator_tag, void, void, void, void>`.
But since this is removed in '20, do we even care about it? I guess not; so this `#if` seems like the minimal change, and therefore OK by me.


================
Comment at: libcxx/include/__memory/raw_storage_iterator.h:41-47
+#if _LIBCPP_STD_VER > 14
+  using iterator_category = output_iterator_tag;
+  using value_type = void;
+  using difference_type = void;
+  using pointer = void;
+  using reference = void;
+#endif
----------------
Here, we //could// drop the `#if` and just unconditionally define these members... except that that would visibly change the value of `raw_storage_iterator::difference_type` (from non-conforming to conforming), and maybe we don't want that? But again I'd think we don't really care about this type and so we might as well lose the `#if`.


================
Comment at: libcxx/include/__memory/raw_storage_iterator.h:50-54
+  _LIBCPP_INLINE_VISIBILITY raw_storage_iterator& operator*() { return *this; }
+  _LIBCPP_INLINE_VISIBILITY raw_storage_iterator& operator=(const _Tp& __element) {
+    ::new ((void*)_VSTD::addressof(*__x_)) _Tp(__element);
+    return *this;
+  }
----------------
I vote for not reformatting here. Keeps the diff smaller.


================
Comment at: libcxx/include/iterator:85
+struct iterator                                            // until C++17
+struct [[deprecated)]] iterator                            // since C++17
 {
----------------
Stray `)` here. But please remove this diff anyway; `[[deprecated]]` doesn't appear in the Standard and even if it did I don't think it needs to appear in the synopsis comments.


================
Comment at: libcxx/include/iterator:629-631
+#if _LIBCPP_STD_VER > 14
+    typedef typename iterator_traits<_Iter>::value_type      value_type;
+#endif
----------------
Lose the `#if` here.
(Any idea why the old code bothered to redundantly redefine `difference_type` etc.?)


================
Comment at: libcxx/test/libcxx/iterators/iterator.requirements/iterator.concepts/cpp20_iter_traits.pass.cpp:45-53
 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*;
----------------
Keep this one inheriting from `iterator_traits`'s primary template and the next one inheriting from `iterator`'s primary template; otherwise you lose the point of the test.
I recommend
```
struct MyIter {
  using iterator_category = std::random_access_iterator_tag;
  using value_type = char;
  using difference_type = std::ptrdiff_t;
  using pointer = char*;
  using reference = char&;
};
struct MyIter2 : MyIter {};
struct MyIter3 : MyIter {};
template<> struct std::iterator_traits<MyIter> : std::iterator_traits<char*> {};
#if TEST_STD_VER <= 17
template<> struct std::iterator_traits<MyIter2> : std::iterator<char*> {};
#endif
```

However, if you just wanted to rip out `MyIter2` for being excessively silly, that might be a better solution.


================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iterator/types.pass.cpp:56
+
+    using iterator_base = std::iterator<typename T::iterator_category, char>;
+#if TEST_STD_VER <= 14
----------------
This isn't right when `T::iterator_category` is `contiguous_iterator_tag`, although maybe the test doesn't care.
This also isn't right when `T::difference_type` isn't `ptrdiff_t`, etc.; but I'm sure the test doesn't care.


================
Comment at: libcxx/test/std/iterators/stream.iterators/istream.iterator/types.pass.cpp:49
 #if TEST_STD_VER <= 14
-    static_assert((std::is_convertible<I1,
-        std::iterator<std::input_iterator_tag, double, std::ptrdiff_t,
-        const double*, const double&> >::value), "");
+    static_assert((std::is_convertible<I1, iterator_base>::value), "");
+    static_assert(std::is_base_of<iterator_base, std::istream_iterator<double> >::value, "");
----------------
Might as well lose the double-parens while you're here. (Presumably a holdover from when `static_assert` was a macro.)


================
Comment at: libcxx/test/std/iterators/stream.iterators/ostreambuf.iterator/types.pass.cpp:29-30
 
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
+
----------------
Throughout: It looks like buildkite isn't happy with some of these pragmas; and I don't really like them either... Aha!
`git grep deprecated libcxx/include/`
`git grep const_fun_mem_t libcxx/test/`
reveals that the correct incantation is
```
#define _LIBCPP_DISABLE_DEPRECATION_WARNINGS
```
at the top of the test. Please make that change throughout.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101729