[libcxx-commits] [PATCH] D116808: fix __simple_view concept in std::ranges

[Hui via Phabricator via libcxx-commits]

huixie90 added a comment.

In D116808#3227900 <https://reviews.llvm.org/D116808#3227900>, @huixie90 wrote:

> @Quuxplusone
>
>> that looks like a libc++ bug that's getting fixed here
>
> The buggy libc++ behaviour is actually better than the correct (by correct I mean same as standard) one. (in my opinion)
>
> Looking at the specification
>
>   constexpr auto begin() requires (!simple-view<V>) {
>     if constexpr (sized_­range<V>) {
>       if constexpr (random_­access_­range<V>) {
>         return ranges::begin(base_);
>       } else {
>         auto sz = range_difference_t<V>(size());
>         return counted_iterator(ranges::begin(base_), sz);
>       }
>     } else {
>       return counted_iterator(ranges::begin(base_), count_);
>     }
>   }
>   
>   constexpr auto begin() const requires range<const V> {
>     if constexpr (sized_­range<const V>) {
>       if constexpr (random_­access_­range<const V>) {
>         return ranges::begin(base_);
>       } else {
>         auto sz = range_difference_t<const V>(size());
>         return counted_iterator(ranges::begin(base_), sz);
>       }
>     } else {
>       return counted_iterator(ranges::begin(base_), count_);
>     }
>   }
>
> Both const and non-const overloads say, hey,  if the `base_` is `random_access_range` and `sized_range`, we can directly use `base_`'s iterator without creating the `counted_iterator`.
>
> For non-const overload of `begin`, it has `requires (!simple-view<V>)`. What I think it means is that, hey, no matter `base_` is `const` or not, they return the same type of `iterators` so we don't need the non-const overload `begin` here, because the `const` version should do the same thing.
>
> But in my contrived example, `V` is `simple-view`, and `sized_range <V>` is true and  `size_range<const V>` is false. so the const overload of `begin` goes to the fallback path. If the non-const overload of `begin` exists (as in the libc++), it would select the better path

That being said, I think the correct fix is to change the standard to be

  constexpr auto begin() requires (!(simple-view<V> && size_range<const V> && random_­access_­range<const V>)) {

so that it can actually return `int*` in this case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116808