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 var-const ldionne
- Group Reviewers
- rG4851fbc3cf23: fix errors on passing input iterator to `std::views::take`
Could you add [libc++] to the commit message?
LGTM % nits. Thanks for the quick fix; sorry that I didn't review it earlier.
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)
clang-format seems to be quite unhappy here. Could you file a bug and fix the formatting for now?
Nit: Use CTAD?
The [[maybe_unused]] seems to be unnecessary.
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).
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.
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>)