[libcxx-commits] [PATCH] D133220: [libc++][ranges] fix errors on passing input iterator to `std::views::take`

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__ranges/take_view.h:229
 template <class _Iter, class _Sent, subrange_kind _Kind>
+  requires requires{typename subrange<_Iter>;}
 struct __passthrough_type<subrange<_Iter, _Sent, _Kind>> {
----------------
philnik wrote:
> I think you said somewhere that GCC was OK without the requires clause. Could you check whether that's a GCC or Clang bug? (If you didn't, just ignore the comment)
The wording is, with simplifications (see https://eel.is/c++draft/range.take#overview-2.2):
```
...if `T` models [...] `ranges​::​subrange`, then [...] `U` is a type determined as follows:
[...]
* otherwise, `T` is a specialization of `ranges​::​subrange`, and `U` is `ranges​::​subrange<iterator_­t<T>>`;
```
Is this a defect in the standard, perhaps? The wording doesn't seem to take into account that `T` modeling `subrange` doesn't guarantee that `iterator_t<T>` models `subrange`. Might be worth asking on the reflector or writing an LWG issue (unless you don't think it's a defect, of course).

I'm also not sure if placing an additional constraint is the best solution here. Wouldn't returning `subrange<_Iter, _Sent>` reflect the intention of the standard better in this case (which is to avoid creating a `take_view` where its effect can easily be applied to the original type or a closely related type)?

Additionally, can you please check what other implementations are doing to inform our decision? From a brief glance, it seems like both GCC and MSVC would return `subrange<_Iter, _Sent>` in this case (though I could easily misread it, a Godbolt would be very helpful).


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/adaptor.pass.cpp:107
 
-  {
-    static_assert(std::same_as<decltype(std::views::take), decltype(std::ranges::views::take)>);
-  }
+  { static_assert(std::same_as<decltype(std::views::take), decltype(std::ranges::views::take)>); }
 
----------------
Nit: it's better to commit reformatting in a separate NFC patch, it makes reviewing easier. But feel free to keep in this patch if splitting it would be too much of a hassle.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/adaptor.pass.cpp:130
   // `views::take(string_view, n)` returns a `string_view`.
-  {
-    {
-      std::string_view sv = "abcdef";
-      std::same_as<decltype(sv)> decltype(auto) result = sv | std::views::take(3);
-      assert(result.size() == 3);
-    }
+  {{std::string_view sv = "abcdef";
+  std::same_as<decltype(sv)> decltype(auto) result = sv | std::views::take(3);
----------------
philnik wrote:
> clang-format seems to be quite unhappy here. Could you file a bug and fix the formatting for now?
+1.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/adaptor.pass.cpp:200
+  std::ranges::subrange<Iter, Sent> r{Iter{input}, Sent{Iter{input + 3}}};
+  [[maybe_unused]] auto tv = std::views::take(std::move(r), 1);
+  auto it = tv.begin();
----------------
philnik wrote:
> The `[[maybe_unused]]` seems to be unnecessary.
+1.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133220