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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

I haven't looked very closely at the tests yet.



================
Comment at: libcxx/docs/Status/Cxx20Papers.csv:111
 "`P1004R2 <https://wg21.link/P1004R2>`__","LWG","Making std::vector constexpr","Cologne","|Complete|","15.0"
-"`P1035R7 <https://wg21.link/P1035R7>`__","LWG","Input Range Adaptors","Cologne","",""
+"`P1035R7 <https://wg21.link/P1035R7>`__","LWG","Input Range Adaptors","Cologne","|In Progress|","16.0"
 "`P1065R2 <https://wg21.link/P1065R2>`__","LWG","Constexpr INVOKE","Cologne","|Complete|","12.0"
----------------
If the paper is in-progress we shouldn't have a release version here. (I know it's that way in a few other places, but the numbers are completely useless)


================
Comment at: libcxx/include/__ranges/istream_view.h:31
+
+#if _LIBCPP_STD_VER > 17
+
----------------
We want to use `>=` for the version checks now. I'd also move the version check outside the `std` namespace. It probably doesn't make much of a difference w.r.t. parsing speed, but it really doesn't hurt either.


================
Comment at: libcxx/include/__ranges/istream_view.h:50
+
+  constexpr default_sentinel_t end() const noexcept { return default_sentinel; }
+
----------------
`_LIBCPP_HIDE_FROM_ABI`


================
Comment at: libcxx/include/__ranges/istream_view.h:56
+  basic_istream<_CharT, _Traits>* __stream_;
+  _Val __value_ = _Val();
+};
----------------
Probably doesn't do much, but doesn't hurt either.


================
Comment at: libcxx/include/__ranges/istream_view.h:104
+struct __fn {
+  template <class _Up, class _UnCVRef = remove_cvref_t<_Up>>
+    requires derived_from<_UnCVRef, basic_istream<typename _UnCVRef::char_type,
----------------
I'm not sure the extra indirection is worth it. Why do you need to strip cv-qualifiers at all?


================
Comment at: libcxx/test/std/ranges/range.factories/range.istream.view/begin.pass.cpp:13
+
+#include <ranges>
+
----------------
AFAIK we don't separate the tested header from the other ones.


================
Comment at: libcxx/test/std/ranges/range.factories/range.istream.view/general.pass.cpp:20
+
+int main(int, const char**) {
+  {
----------------



================
Comment at: libcxx/test/std/ranges/range.factories/range.istream.view/iterator/deref.pass.cpp:39
+}
\ No newline at end of file

----------------
missing newline


================
Comment at: libcxx/test/std/ranges/range.factories/range.istream.view/iterator/member_types.compile.pass.cpp:24
+  static_assert(std::is_same_v<typename Iter::value_type, Val>);
+  static_assert(!HasIterCategory<Iter>);
+}
----------------
Could you also test a valid one?


================
Comment at: libcxx/test/std/ranges/range.factories/range.istream.view/range.concept.compile.pass.cpp:15
+
+template <class Val, class CharT, class Traits = std::char_traits<CharT>>
+concept HasIstreamView = requires { typename std::ranges::basic_istream_view<Val, CharT, Traits>; };
----------------
Why don't you test `Traits` in any way?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133317