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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

I didn't do a review, but mainly had a look why the CI fails.



================
Comment at: libcxx/include/module.modulemap.in:1016
       module iota_view              { private header "__ranges/iota_view.h" }
+      module istream_view           { private header "__ranges/istream_view.h" }
       module join_view              { private header "__ranges/join_view.h" }
----------------
See the comment in `ranges` why this should be disabled when localization is disabled.


================
Comment at: libcxx/include/ranges:304
 #include <__ranges/iota_view.h>
+#include <__ranges/istream_view.h>
 #include <__ranges/join_view.h>
----------------
Maybe move this to the end. This header "works", but can't be used when localization isn't enabled. The header includes `<iosfwd>` which is fine when no locales are used. However when using the "contents" of the `istream_view.h` header you need to include `<istream>`, which indirectly includes `<ios>` and results in an error in that header on line 216.

So I think it would be best to not provide this header when localization is disable.

I use a similar approach in `<chrono>` where I don't include headers when localization is disabled. (There are more conditions there, but localization is a reason on itself.)


================
Comment at: libcxx/test/libcxx/ranges/range.factories/range.istream.view/no_unique_address.compile.pass.cpp:9
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
----------------
The same for the other tests.


================
Comment at: libcxx/test/std/ranges/range.factories/range.istream.view/begin.pass.cpp:25
+
+#ifndef TEST_HAS_NO_WIDE_CHARACTERS
+static_assert(HasBegin<std::ranges::wistream_view<int>>);
----------------
This macro is never defined since "test_macros.h" isn't included.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133317