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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const marked 6 inline comments as done.
var-const added a comment.

In D119620#3317884 <https://reviews.llvm.org/D119620#3317884>, @Quuxplusone wrote:

> I suggest fixing this by changing `<__iterator/default_sentinel.h>` to enable `default_sentinel` in C++20 regardless of `_LIBCPP_HAS_NO_CONCEPTS` — it should merely be guarded by `#if _LIBCPP_STD_VER > 17` and nothing else.

Done. Since it doesn't actually use concepts, that seems reasonable.



================
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_;
+    }
----------------
Quuxplusone wrote:
> 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.)
Sure.


================
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);
----------------
Quuxplusone wrote:
> `operator!=` should be under `#if _LIBCPP_STD_VER <= 17`, because it's gone in '20.
Done (I don't think this is related to the One Ranges proposal, but it makes sense to do these changes in one go). Also mentioned this in the synopsis.


================
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
----------------
Quuxplusone wrote:
> Nit: To match the synopsis comment, and the `throw()/noexcept` distinction isn't terribly important IMHO.
Done. I've done the same change in the synopsis -- you intended to change both, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119620