[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 accepted this revision.
var-const added inline comments.
This revision is now accepted and ready to land.


================
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>> {
----------------
huixie90 wrote:
> var-const wrote:
> > 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).
> 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>`)
> 
You're right -- sorry, I missed the `random_access_range` part. In that case, it looks like this is the right fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133220