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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

This looks pretty good!



================
Comment at: libcxx/docs/Status/RangesIssues.csv:1
 "Number","Name","Status","First released version"
 `P0896R4 <https://wg21.link/P0896R4>`__,<ranges>,|Complete|,15.0
----------------
Not for this review: I think we should get rid of this file entirely, since it's only a duplicate of information we already have in `CXXYZIssues.csv`?


================
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>`_,,,
----------------
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?


================
Comment at: libcxx/include/__ranges/istream_view.h:42
+public:
+  _LIBCPP_HIDE_FROM_ABI constexpr explicit basic_istream_view(basic_istream<_CharT, _Traits>& __stream)
+      : __stream_(std::addressof(__stream)) {}
----------------
Can you confirm whether you have a test that this is explicit?


================
Comment at: libcxx/include/__ranges/istream_view.h:50
+
+  _LIBCPP_HIDE_FROM_ABI constexpr default_sentinel_t end() const noexcept { return default_sentinel; }
+
----------------
Do you have a test that this is `noexcept`?


================
Comment at: libcxx/include/__ranges/istream_view.h:71
+  __iterator(const __iterator&)                  = delete;
+  _LIBCPP_HIDE_FROM_ABI __iterator(__iterator&&) = default;
+
----------------
I think this is a great "catch", in the sense that we usually forget to use `_LIBCPP_HIDE_FROM_ABI` on `= default` functions, but we should.


================
Comment at: libcxx/include/__ranges/istream_view.h:100-101
+
+template <class _Val>
+using wistream_view = basic_istream_view<_Val, wchar_t>;
+
----------------
```
#ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
...
#endif
```

You will probably need to update the tests with that in mind, too? The test macro to use is:

```
#ifndef TEST_HAS_NO_WIDE_CHARACTERS
...
#endif
```

If you have a test that is *only* meaningful for wide characters (e.g. the test file is only checking `wchar_t` related stuff), then you can use `// UNSUPPORTED: no-wide-characters` to disable the whole test in that case. That's better than wrapping the whole test with `#ifndef TEST_HAS_NO_WIDE_CHARACTERS`.

This is tested in the CI, so I would assume this would have failed in the CI (although our CI is not perfect in that regard because it uses a platform that actually provides `wchar_t` but we make it an error to include `<wchar.h>`).


================
Comment at: libcxx/include/__ranges/istream_view.h:112
+                                                  typename _UnCVRef::traits_type>>
+  _LIBCPP_HIDE_FROM_ABI auto operator()(_Up&& __u) const
+    noexcept(noexcept(basic_istream_view<_Tp, typename _UnCVRef::char_type,
----------------
I *think* this should be `constexpr`, although I'm not sure this is observable.


================
Comment at: libcxx/test/std/ranges/range.factories/range.istream.view/begin.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
General comment that applies to (probably) all tests: let's make sure we also test `wchar_t` and `wistream_view`. You should probably templatize your tests on `CharT` and use a mechanism similar to what @Mordante does for `format`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133317