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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__iterator/istream_iterator.h:50
+#if !defined(_LIBCPP_HAS_NO_CONCEPTS)
+    _LIBCPP_HIDE_FROM_ABI constexpr istream_iterator(default_sentinel_t) : istream_iterator() {}
+#endif // !defined(_LIBCPP_HAS_NO_CONCEPTS)
----------------
Note: the default constructor and this constructor are defined in the same clause in the Standard (`23.6.2.2.1`, http://eel.is/c++draft/istream.iterator#cons-1), so it made sense to define one in terms of the other.


================
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);
+    }
----------------
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.


================
Comment at: libcxx/test/std/iterators/stream.iterators/istream.iterator/istream.iterator.cons/default.pass.cpp:57
+    {
+    typedef std::istream_iterator<int> T;
+
----------------
Because these constructors (default constructor and `default_sentinel_t` constructor) are defined in the same clause, I'm leaning towards testing them in the same test file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119620