[libc++][ranges] fix errors on passing input iterator to `std::views::take`

[huixie90]

Summary

In the implementation of std::views::take, it uses subrange<Iter> as part of the return type. But in case of input iterator, subrange<Iter> can be ill-formed

[philnik]

Could you add [libc++] to the commit message?
LGTM % nits. Thanks for the quick fix; sorry that I didn't review it earlier.

libcxx/include/__ranges/take_view.h

229

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)

libcxx/test/std/ranges/range.adaptors/range.take/adaptor.pass.cpp

136

clang-format seems to be quite unhappy here. Could you file a bug and fix the formatting for now?

207

Nit: Use CTAD?

208

The [[maybe_unused]] seems to be unnecessary.

[var-const]

libcxx/include/__ranges/take_view.h

229

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).

libcxx/test/std/ranges/range.adaptors/range.take/adaptor.pass.cpp

108

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.

136

+1.

208

+1.

[huixie90]

addressed comments

[huixie90]

libcxx/include/__ranges/take_view.h

229

I don't think it is a defect in the standard. That paragraph has a condition "if T models random_­access_­range". In case of random_­access_­range, subrange<Iter> is always valid. But in this particular case of input_iterator, subrange<Iter> is invalid but it should not pass the "if T models random_­access_­range" condition, and should fall through https://eel.is/c++draft/range.take#overview-2.5

The standard is correct, in case of random_access_range, it should return subrange<Iter>, i.e. subrange<Iter, Iter>, because we need to return subrange(it, it + n) (both are iterators not sentinel). In case of input_iterator, it should simply fall under the last paragraph and return take_view (it should never return subrange<Iter, Sent>)

[var-const]

libcxx/include/__ranges/take_view.h

229

You're right -- sorry, I missed the random_access_range part. In that case, it looks like this is the right fix.

libcxx/include/__ranges/take_view.h

 };
 
 template <class _CharT, class _Traits>
 struct __passthrough_type<basic_string_view<_CharT, _Traits>> {
   using type = basic_string_view<_CharT, _Traits>;
 };
 
 template <class _Iter, class _Sent, subrange_kind _Kind>
+  requires requires{typename subrange<_Iter>;}
 struct __passthrough_type<subrange<_Iter, _Sent, _Kind>> {
   using type = subrange<_Iter>;
 };
 
 template <class _Tp>
 using __passthrough_type_t = typename __passthrough_type<_Tp>::type;
 
 struct __fn {

libcxx/test/std/ranges/range.adaptors/range.take/adaptor.pass.cpp

// Check SFINAE friendliness
       static_assert(!CanBePiped<NotAView,    decltype(std::views::take(3))>);
 
       static_assert(!CanBePiped<SomeView&,   decltype(std::views::take(/*n=*/NotAView{}))>);
     }
   }
 
   {
     static_assert(std::same_as<decltype(std::views::take), decltype(std::ranges::views::take)>);
   }
 
   // `views::take(empty_view, n)` returns an `empty_view`.
   {
     using Result = std::ranges::empty_view<int>;
     [[maybe_unused]] std::same_as<Result> decltype(auto) result = std::views::empty<int> | std::views::take(3);
   }
 
   // `views::take(span, n)` returns a `span`.
constexpr bool test() {
   }
 
   // `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::u32string_view sv = U"abcdef";
       std::same_as<decltype(sv)> decltype(auto) result = sv | std::views::take(3);
       assert(result.size() == 3);
     }
   }
 
constexpr bool test() {
 
   // Test that it's possible to call `std::views::take` with any single argument as long as the resulting closure is
   // never invoked. There is no good use case for it, but it's valid.
   {
     struct X { };
     [[maybe_unused]] auto partial = std::views::take(X{});
   }
 
+  // Test when `subrange<Iter>` is not well formed
+  {
+    int input[] = {1, 2, 3};
+    using Iter  = cpp20_input_iterator<int*>;
+    using Sent  = sentinel_wrapper<Iter>;
+    std::ranges::subrange r{Iter{input}, Sent{Iter{input + 3}}};
+    auto tv = std::views::take(std::move(r), 1);
+    auto it                  = tv.begin();
+    assert(*it == 1);
+    ++it;
+    assert(it == tv.end());
+  }
+
   return true;
 }
 
 int main(int, char**) {
   test();
   static_assert(test());
 
   return 0;
 }