[libcxx-commits] [PATCH] D119620: [libc++][ranges] Implement Ranges changes to `istream{, buf}_iterator`.

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

Quuxplusone accepted this revision.
Quuxplusone added a comment.
This revision is now accepted and ready to land.

LGTM % minor comments!
Please fix (especially the `operator!=`s), rebase and re-upload to see a green CI run (now that the ASAN failures should be fixed, right?), but then this seems ready to ship.



================
Comment at: libcxx/include/__iterator/istream_iterator.h:77
+    friend _LIBCPP_HIDE_FROM_ABI bool operator==(const istream_iterator& __i, default_sentinel_t) {
+      return !__i.__in_stream_;
+    }
----------------
Nit: I know the standard says `!in_stream`, but can we please make this `return __i.__in_stream_ == nullptr;` for clarity? (Since it's just a raw pointer, there's no difference except readability.)


================
Comment at: libcxx/include/__iterator/istream_iterator.h:91-98
 template <class _Tp, class _CharT, class _Traits, class _Distance>
 inline _LIBCPP_INLINE_VISIBILITY
 bool
 operator!=(const istream_iterator<_Tp, _CharT, _Traits, _Distance>& __x,
            const istream_iterator<_Tp, _CharT, _Traits, _Distance>& __y)
 {
     return !(__x == __y);
----------------
`operator!=` should be under `#if _LIBCPP_STD_VER <= 17`, because it's gone in '20.


================
Comment at: libcxx/include/__iterator/istreambuf_iterator.h:70
+#if !defined(_LIBCPP_HAS_NO_CONCEPTS)
+    _LIBCPP_INLINE_VISIBILITY constexpr istreambuf_iterator(default_sentinel_t) _NOEXCEPT
+        : istreambuf_iterator() {}
----------------



================
Comment at: libcxx/include/__iterator/istreambuf_iterator.h:108-112
 template <class _CharT, class _Traits>
 inline _LIBCPP_INLINE_VISIBILITY
 bool operator!=(const istreambuf_iterator<_CharT,_Traits>& __a,
                 const istreambuf_iterator<_CharT,_Traits>& __b)
                 {return !__a.equal(__b);}
----------------
As above, this `operator!=` is gone in '20.


================
Comment at: libcxx/include/__iterator/istreambuf_iterator.h:97
+    friend _LIBCPP_HIDE_FROM_ABI bool operator==(const istreambuf_iterator& __i, default_sentinel_t __s) {
+      return __i.equal(__s);
+    }
----------------
var-const wrote:
> This is the definition from the Standard verbatim (http://eel.is/c++draft/istreambuf.iterator#ops-7). At least conceptually, it involves creating a `istreambuf_iterator` from the sentinel. We could instead do something like `return __test_for_eof();`. I'm not sure if it's worth it, though.
I agree; but I think it //is// worth it, just to get rid of a layer of indirection and avoid having to name the `__s` parameter. :) `return __i.__test_for_eof();` sounds like a win to me.


================
Comment at: libcxx/test/std/iterators/stream.iterators/istreambuf.iterator/istreambuf.iterator.cons/default.pass.cpp:13-14
 //
-// istreambuf_iterator() throw();
+// istreambuf_iterator() throw; // until C++11
+// constexpr istreambuf_iterator() noexcept; // since C++11
+// constexpr istreambuf_iterator(default_sentinel_t) noexcept; // since C++20
----------------
Nit: To match the synopsis comment, and the `throw()/noexcept` distinction isn't terribly important IMHO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119620