[libcxx-commits] [PATCH] D133317: [libc++][ranges] implement `std::views::istream`

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const accepted this revision as: var-const.
var-const added a comment.

I have just a few nitpicks; LGTM once the existing feedback is addressed.



================
Comment at: libcxx/docs/Status/RangesPaper.csv:162
 `[range.reverse] <https://wg21.link/range.reverse>`_,`reverse_view <https://llvm.org/D107096>`_,[range.all],Zoe Carver,✅
+`[range.istream] <https://wg21.link/range.istream>`_,`istream_view <https://llvm.org/D133317>`_,,Hui Xie,✅
+`[range.take.while] <https://wg21.link/range.take.while>`_,`take_while_view <https://llvm.org/DXXXXXX>`_,,,
----------------
ldionne wrote:
> The plan was also to get rid of this file once the One Ranges Proposal had been completed. It was basically used to track sub parts of the One Ranges Proposal.
> 
> @var-const WDYT, can we remove this now?
Yes, I'll do it next week.


================
Comment at: libcxx/include/__ranges/istream_view.h:56
+  basic_istream<_CharT, _Traits>* __stream_;
+  _LIBCPP_NO_UNIQUE_ADDRESS _Val __value_ = _Val();
+};
----------------
Can you add a libc++-specific test for this optimization?


================
Comment at: libcxx/test/std/ranges/range.factories/range.istream.view/iterator/increment.pass.cpp:48
+  }
+}
----------------
Nit: `return 0;` (here and elsewhere).


================
Comment at: libcxx/test/std/ranges/range.factories/range.istream.view/iterator/member_types.compile.pass.cpp:15
+
+
+template <class T>
----------------
Nit: unnecessary blank line.


================
Comment at: libcxx/test/std/ranges/range.factories/range.istream.view/range.concept.compile.pass.cpp:20
+
+// movable Val
+struct UnMovable {
----------------
Nit: `unmovable Val`?


================
Comment at: libcxx/test/std/ranges/range.factories/range.istream.view/range.concept.compile.pass.cpp:21
+// movable Val
+struct UnMovable {
+  UnMovable()            = default;
----------------
Ultranit: `s/UnMovable/Unmovable/`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133317