[libc++] fix __simple_view concept in std::ranges

[huixie90]

Summary

fix __simple_view concept in std::ranges

For reference, the definition in the standard is here:
http://eel.is/c++draft/ranges#range.utility.helpers

Changes:

libcxx/include/__ranges/concepts.h

[• Quuxplusone]

Please undo your formatting changes, so that the code change is easier to find in the diff (and also because the old formatting was strictly better). I actually don't see what your proposed difference is right now at all — it looks identical?

[philnik]

Please add a regression test to test/libcxx/ranges/range.utility.helpers/simple_view.compile.pass
@Quuxplusone It took me some time too. The second same_as is changed.

[huixie90]

In D116808#3227463, @Quuxplusone wrote:

Please undo your formatting changes, so that the code change is easier to find in the diff (and also because the old formatting was strictly better). I actually don't see what your proposed difference is right now at all — it looks identical?

I am not sure about the process and I am happy to undo the formatting change. But I followed the instructions of the contribution guide and used the "arc" tool for the review and the tool suggested me to lint the code in this way and automatically applied the formatting fix.

the actual change is the last bit is changed from iterator_t -> sentinel_t

anyway. should I surround the code clang-format off with clang-format on after revert the formatting?

[philnik]

In D116808#3227470, @huixie90 wrote:

In D116808#3227463, @Quuxplusone wrote:

Please undo your formatting changes, so that the code change is easier to find in the diff (and also because the old formatting was strictly better). I actually don't see what your proposed difference is right now at all — it looks identical?

I am not sure about the process and I am happy to undo the formatting change. But I followed the instructions of the contribution guide and used the "arc" tool for the review and the tool suggested me to lint the code in this way and automatically applied the formatting fix.

the actual change is the last bit is changed from iterator_t -> sentinel_t

anyway. should I surround the code clang-format off with clang-format on after revert the formatting?

Just use --nolint with arc. Don't add //clang-format. Almost no file in libc++ is conforming to the clang-format rules. It is not the most useful in libc++, because we have much weird stuff which clang-format just doesn't format properly.

[miscco]

libcxx/include/__ranges/concepts.h

89

If you want to you can also give clang format a nudge by adding // where you want a line break

That said I do not know whether this is usually accepted here

[• Quuxplusone]

Ah, I see; thanks @philnik! This looks like a real bugfix then. Thankfully, it looks like we have test coverage for this case — so this PR also needs to update some existing tests, but might not need to add many (if any) new tests.

OTOH, it looks like __simple_view is used inside take_view, drop_view, and join_view, and none of those tests are failing. This indicates missing test coverage for those view adaptors.

libcxx/include/__ranges/concepts.h

89

It's not usually accepted to commit code with random empty // comments. :)
Anyway, in this case the right answer is "don't run clang-format."

[huixie90]

Updating D116808: fix __simple_view concept in std::ranges

revert formatting and add unit test

[huixie90]

Updating D116808: fix __simple_view concept in std::ranges

remove white space

[philnik]

Please also add test coverage in take_view, drop_view, and join_view, which @Quuxplusone basically requested, unless I'm mistaken.

libcxx/test/libcxx/ranges/range.utility.helpers/simple_view.compile.pass.cpp

37

IIUC an iterator is also a sentinel, so Different makes more sense than With.

51–52

Remove the space in line 48 too (which I would prefer) or put here a space (which @Quuxplusone would probably prefer), so put here a whitespace in.

[huixie90]

@philnik, @Quuxplusone
I can't find any test files that contain drop take and join.
I guess those views are completely untested unless I miss something. If this is the case, would it be a separate task to add complete test suites for these views?

Anyways, I found an interesting behaviour change after the update
https://godbolt.org/z/fzvWoh4xE

[philnik]

The tests are in libcxx/test/std/ranges/range.adaptors/{range.take, range.drop, range.join.view}.

[• Quuxplusone]

Anyways, I found an interesting behaviour change after the update

(Slightly simplified: https://godbolt.org/z/8MnoGzWaM ) Yep, that looks like a libc++ bug that's getting fixed here, so please also add a regression test for that, in the take_view tests.
When you searched for test coverage, I'm guessing you grepped for views::take. libc++ doesn't implement that CPO yet. All we have implemented so far is the ranges::take_view type. But I believe @ldionne is working on views::take, because he mentioned it after I'd also posted it in D115122 (but D115122 lacks tests).

$ git grep views::take ../libcxx/test/
../libcxx/test/std/library/description/conventions/customization.point.object/cpo.compile.pass.cpp://static_assert(test(std::views::take, a, 10));
../libcxx/test/std/library/description/conventions/customization.point.object/cpo.compile.pass.cpp://static_assert(test(std::views::take_while, a, [](int x){ return x < 10; }));
$ git grep ranges::take_view ../libcxx/test/ | wc -l
      55

libcxx/test/libcxx/ranges/range.utility.helpers/simple_view.compile.pass.cpp

51–52

Yep, let's keep these aligned by adding the space here.

[huixie90]

@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

[huixie90]

In 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

[• Quuxplusone]

@huixie90: FYI, I brought this idea up in the #ranges channel on Slack; we'll see what the experts say. :) https://cpplang.slack.com/archives/C4Q3A3XB8/p1641579035002100

[huixie90]

Updating D116808: fix __simple_view concept in std::ranges

added unit test for take_view

[huixie90]

@Quuxplusone

I added tests for take_view as it has observable behaviour change. For other views, I can't seem to find observable things. As simple-view concept is really about removing the non-const overload if it is same as the const overload. The only thing I am thinking of testing is that if simple-view<BaseView> is satisfied, we can check if it only has one overload of begin. I tried different things
https://godbolt.org/z/W6vsjdG95
https://godbolt.org/z/jbnKf4Yqv
https://godbolt.org/z/j6GP1bMnb

But compilers don't all agree and in one case, it even triggers clang to ICE

[huixie90]

Updating D116808: fix __simple_view concept in std::ranges

add unit tests for drop_view and join_view

[huixie90]

Updating D116808: fix __simple_view concept in std::ranges

[huixie90]

Updating D116808: fix __simple_view concept in std::ranges

update take test, no need to instantiate the class

[huixie90]

Updating D116808: fix __simple_view concept in std::ranges

remove unnamed namespace to work around -Wundefined-internal

[• Quuxplusone]

libcxx/test/std/ranges/range.adaptors/range.drop/begin.pass.cpp

85–97 ↗

(On Diff #398342)

{
  bool called = false;
  auto dv = std::ranges::drop_view<SimpleView>(SimpleView{&called}, 4);
  assert(std::as_const(dv).begin() == nullptr);
  assert(!called);
  assert(dv.begin() == nullptr);
  assert(called);
}

You don't want to store bool called inside the view object itself, because the view object itself is going to get copied around. You want to store the bool outside the view object, and let the view object copy around a pointer to the bool.

Also, as below, please move the comments into static_asserts circa lines 37 and 45.

libcxx/test/std/ranges/range.adaptors/range.join.view/begin.pass.cpp

102–106 ↗

(On Diff #398342)

{
  std::ranges::join_view<ParentView<ChildView>> jv;
  ASSERT_SAME_TYPE(decltype(jv.begin()), ???);
  ASSERT_SAME_TYPE(decltype(std::as_const(jv).begin()), ???);
}
{
  std::ranges::join_view<SimpleParentView> jv;
  ASSERT_SAME_TYPE(decltype(jv.begin()), ???);
  ASSERT_SAME_TYPE(decltype(std::as_const(jv).begin()), ???);
}

(filling in the actual expected types)

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

80–81

{
  std::ranges::take_view<NonCommonSimpleView> tv;
  ASSERT_SAME_TYPE(decltype(tv.begin()), std::counted_iterator<int*>);
  ASSERT_SAME_TYPE(decltype(std::as_const(tv).begin()), std::counted_iterator<int*>);
}

[huixie90]

@Quuxplusone

You don't want to store bool called inside the view object itself, because the view object itself is going to get copied around. You want to store the bool outside the view object, and let the view object copy around a pointer to the bool.

I could do but I don't think it is necessary.
First, the test is checking the view instance stored inside the drop_view via base function, which returns by reference in the specification . It is not checking any other instances. correction -> the base function returns by copy or move. but that doesn't matter because the copy is made after I called begin
Second, unlike iterators, view object is not going to be copied around because 1. view concept only requires movable 2. algorithms cannot assume views are cheap to copy (constant time doesn't mean cheap). Iterators are cheap to copy.

please move the comments into static_asserts circa lines 37 and 45.

Sure I can do. I put comments before the test points just because all other test points do the same thing. but happy to move

(filling in the actual expected types)

Iterators are private defined inside the view. How can you spell it out without using decltype?

ASSERT_SAME_TYPE(decltype(std::as_const(tv).begin()), std::counted_iterator<int*>);

The reason I don't create an instance is that I think I did create an instance at beginning and some configuration in the CI failed and complained that some of my member function is not defined. what about

{
    using TakeNonSimpleView = std::ranges::take_view<NonCommonSimpleView>;
    ASSERT_SAME_TYPE(decltype(std::declval<TakeNonSimpleView&>().begin()), std::counted_iterator<int*>);
    ASSERT_SAME_TYPE(decltype(std::declval<TakeNonSimpleView const&>().begin()), std::counted_iterator<int*>);
}

[huixie90]

@Quuxplusone
in addition to the previous comment , the suggested code is actually not correct according to the standard

{
  bool called = false;
  auto dv = std::ranges::drop_view<SimpleView>(SimpleView{&called}, 4);
  assert(std::as_const(dv).begin() == nullptr);
  assert(!called);
  assert(dv.begin() == nullptr);
  assert(called);  // this is going to fail because according to the standard the non-const overload of begin doesn't exist if `base` is `simple-view`. so `called` will still be false here
}

[• Quuxplusone]

libcxx/test/std/ranges/range.adaptors/range.drop/begin.pass.cpp

85–97 ↗

(On Diff #398342)

First, the test is checking the view instance stored inside the drop_view via base function, which returns by reference in the specification. It is not checking any other instances.

True, but it'll still be much clearer to check a bool called that we created ourselves right here in this test.

Second, unlike iterators, view object is not going to be copied around because 1. view concept only requires movable 2. algorithms cannot assume views are cheap to copy (constant time doesn't mean cheap).

When I said "copied around," I include "moved around" in that, too. ;) Even if the SimpleView was merely moved into dv, or if base() returned by moving out of it, that would still result in multiple bool objects in the system, which makes it more-difficult-than-it-needs-to-be to tell whether we're checking the value of the correct bool object. If there's just one bool, and (possibly a bunch of) pointers that point to it, that's much easier to do. Also constexpr-friendly. We use this pattern in a lot of places in the tests; we should use it here too.

actually we expect called to be false in both cases

Ah, right. Please add a similarly structured test, right next to this one, in which we expect called to be false-and-then-true. The only difference between the tests should be that one of the view types has size_t size() { return 0; } and the other has size_t size() const { return 0; }. Actually, I suspect that you could define the types directly inside this function instead of many lines above. Something like this:

{
  struct V : std::ranges::view_base {
    int *p = nullptr;
    constexpr int *begin() { *p += 1; return nullptr; }
    constexpr int *begin() const { *p += 10; return nullptr; }
    constexpr int *end() const { return nullptr; }
    constexpr size_t size() const { return 0; }
  };
  static_assert(std::random_access_range<V> && std::view<V>);
  static_assert(std::sized_range<V> && std::sized_range<const V>);
  LIBCPP_STATIC_ASSERT(std::__simple_range<V>);
  int calls = 0;
  auto dv = std::ranges::drop_view<V>(V{&calls}, 1);
  assert(std::as_const(dv).begin() == nullptr);
  assert(calls == 10);
  assert(dv.begin() == nullptr);
  assert(calls == 20);
}
{
  struct V : std::ranges::view_base {
    int *p = nullptr;
    constexpr int *begin() { *p += 1; return nullptr; }
    constexpr int *begin() const { *p += 10; return nullptr; }
    constexpr int *end() const { return nullptr; }
    constexpr size_t size() { return 0; }  // deliberately non-const
  };
  static_assert(std::random_access_range<V> && std::view<V>);
  static_assert(std::sized_range<V> && !std::sized_range<const V>);
  LIBCPP_STATIC_ASSERT(std::__simple_range<V>);
  int calls = 0;
  auto dv = std::ranges::drop_view<V>(V{&calls}, 1);
  assert(std::as_const(dv).begin() == nullptr);
  assert(calls == 10);
  assert(dv.begin() == nullptr);
  assert(calls == 11);
}

libcxx/test/std/ranges/range.adaptors/range.join.view/begin.pass.cpp

102–106 ↗

(On Diff #398342)

Iterators are private defined inside the view. How can you spell it out without using decltype?

Ah, I see. Okay, this is fine then. Or at least it'll be fine as

{
  std::ranges::join_view<ParentView<ChildView>> jv;
  const std::ranges::join_view<ParentView<ChildView>> cjv;
  static_assert(std::same_as<decltype(jv.begin()), decltype(cjv.begin())>);
}
{
  std::ranges::join_view<SimpleParentView> jv;
  const std::ranges::join_view<SimpleParentView> cjv;
  static_assert(!std::same_as<decltype(jv.begin()), decltype(cjv.begin())>);
}

Stylistically, I'd still like to avoid declval as much as possible — and here it's possible, because we're dealing entirely with types that are easy to default-construct.

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

80–81

The reason I don't create an instance is that I think I did create an instance at beginning and some configuration in the CI failed and complained that some of my member function is not defined.

Ah, I see. But then you can just define them: change line 25

int* begin() const;

to

int *begin() const { return nullptr; }

and so on.

[huixie90]

Updating D116808: fix __simple_view concept in std::ranges

addressed issues in the code review. updated the tests

[huixie90]

@Quuxplusone

Please add a similarly structured test, right next to this one, in which we expect called to be false-and-then-true. The only difference between the tests should be that one of the view types has size_t size() { return 0; } and the other has size_t size() const { return 0; }.

I added a false then true case test, but without adding size const overload. Instead, I simply made it not satisfying simple view concept. (which is the concept I really want to test)

libcxx/test/std/ranges/range.adaptors/range.drop/begin.pass.cpp

85–97 ↗

(On Diff #398342)

that sounds good . i will update then

libcxx/test/std/ranges/range.adaptors/range.join.view/begin.pass.cpp

102–106 ↗

(On Diff #398342)

yes. i can do that. the only thing i don't like is std::ranges::join_view<ParentView<ChildView>> is not default constructible are is very hard to construct one. I might just write another struct that can be default initialised

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

80–81

sounds good

[ldionne]

@huixie90 Thanks for the fix. Since this already has some people involved, I won't chime in more than to clarify a few things:

In D116808#3227470, @huixie90 wrote:

I am not sure about the process and I am happy to undo the formatting change. But I followed the instructions of the contribution guide and used the "arc" tool for the review and the tool suggested me to lint the code in this way and automatically applied the formatting fix.

Don't worry -- it's indeed very misleading. This is a frequent issue for contributors, and we should make it so that arc doesn't try to run lint by default on libcxx/ and libcxxabi/. But I don't know how to make that happen (which also points out to another problem, which is the high barrier to LLVM's infrastructure).

In D116808#3227888, @Quuxplusone wrote:

When you searched for test coverage, I'm guessing you grepped for views::take. libc++ doesn't implement that CPO yet. All we have implemented so far is the ranges::take_view type. But I believe @ldionne is working on views::take, because he mentioned it after I'd also posted it in D115122 (but D115122 lacks tests).

Indeed, I have local patches I'm slowly finishing up in the middle of everything else. They add views::take, views::drop and views::join. Those views are tested though, just not their range adaptor objects.

[huixie90]

@Quuxplusone,

What would be the next step here? I think I'd need someone says "ship it" before I can continue.

[• Quuxplusone]

LGTM % these last few comments.

@huixie90, please wait for a second approval from either @Mordante or @ldionne before you land this. Whoever-it-is will put a checkmark next to "Group Reviewer libc++" above.

libcxx/test/std/ranges/range.adaptors/range.drop/begin.pass.cpp

42 ↗

(On Diff #398371)

90 ↗

(On Diff #398371)

99–100 ↗

(On Diff #398371)

103 ↗

(On Diff #398371)

(Aside: That extra {} is an initializer for the empty base class view_base? Yuck. But if the language requires it... okay.)

104 ↗

(On Diff #398371)

My proposed version ( https://reviews.llvm.org/D116808#3229481 ) used just a single int calls so that we could write calls == 10, calls == 11 in one line, and I think it ends up being a bit cleaner. But this is OK. I'm just asking that we make sure to check the right number of calls after each call to begin.

libcxx/test/std/ranges/range.adaptors/range.join.view/begin.pass.cpp

29–30 ↗

(On Diff #398371)

Here and lines 23–25, if these functions are never called, please remove their definitions.

const ChildView* begin() const;
const ChildView* end() const;

If they are called, and I just missed it, then of course this is fine.
(And if they're not called, then the fact that I just spent a couple minutes looking for calls that don't exist should explain why I'm asking for the definitions to be removed!)

109 ↗

(On Diff #398371)

Here and line 102: remove the {}.

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

24–30

struct NonCommonSimpleView : std::ranges::view_base {
  int* begin() const;
  sentinel_wrapper<int*> end() const;
  size_t size() { return 0; }  // deliberately non-const
};
static_assert(std::ranges::sized_range<DifferentSentinel>);
static_assert(!std::ranges::sized_range<const DifferentSentinel>);

(There are several changes here; please copy-paste. I originally submitted this as a "Suggest Edit," but Phabricator made the diff really confusing.)

80–81

On line 76, please remove the {}.

[huixie90]

Updating D116808: fix __simple_view concept in std::ranges

addressed issues in the review

[huixie90]

@Quuxplusone I applied the suggestions and see if it compiles fine.

libcxx/test/std/ranges/range.adaptors/range.drop/begin.pass.cpp

103 ↗

(On Diff #398371)

Yes, it wouldn't compile without the extra {}

104 ↗

(On Diff #398371)

yeah I think they are doing the same thing. I prefer separate variables. I have to look at the implementation to figure out what calls==10 really means. will make sure after each call to begin, the numbers are checked

libcxx/test/std/ranges/range.adaptors/range.join.view/begin.pass.cpp

29–30 ↗

(On Diff #398371)

It was not called. It was my desperate attempt to workaround this ci failure
https://reviews.llvm.org/harbormaster/unit/view/1774321/

will remove and try again. This time I removed unnamed namespace and hopefully that error will go. (btw, I think it is still a good idea to stop defining symbols in global namespace even though each TU has its own main function and not supposed to link against anything.)

109 ↗

(On Diff #398371)

removed. but is there a reason why this is recommended? I thought brace initialisation is recommended in almost all cases (except the case you don't want the std::initializer_list). From a user point of view, if you don't know if a type is an aggregate like thing or it has proper default constructor, using {} will always initialise it.

[ldionne]

Please consider starting your commit message (the short title line) with [libc++]. You don't *have* to, but it's customary for us to do it.

[• Quuxplusone]

libcxx/test/std/ranges/range.adaptors/range.join.view/begin.pass.cpp

29–30 ↗

(On Diff #398371)

This time I removed unnamed namespace and hopefully that error will go.

Yup. :) Occam's Razor: don't do something unless you need to. Here, adding an extra namespace { (or static) resulted in the need to add an extra { return nullptr; }. Removing the one lets you remove the other. And this explains why our tests generally don't make things internal-linkage (unless that's relevant to what's being tested!).

109 ↗

(On Diff #398371)

I thought brace initialisation is recommended in almost all cases (except the case you don't want the std::initializer_list)

You basically never want the std::initializer_list (I mean, you want it only when you're initializing a container; and here jv is not a container). My common-sense guidelines are here:
https://quuxplusone.github.io/blog/2019/02/18/knightmare-of-initialization/#simple-guidelines-for-variable-i

• Use = whenever you can.
• Use initializer-list syntax {} only for element initializers (of containers and aggregates).
• Use function-call syntax () to call a constructor, viewed as an object-factory.

I'm pleased to say that libc++'s codebase mostly follows these guidelines by default — via convergent evolution, rather than any effort of my own.

[huixie90]

@ldionne I don't seem to have access to push to llvm.
This is the link I followed
https://llvm.org/docs/Phabricator.html#committing-someone-s-change-from-phabricator

and I get this error when I try to push
remote: Permission to llvm/llvm-project.git denied to huixie90.
fatal: unable to access 'https://github.com/llvm/llvm-project.git/': The requested URL returned error: 403

[ldionne]

If you need someone to commit this on your behalf, please provide Author Name <email@domain> for attribution and we'll do it.

[huixie90]

@ldionne:

Thanks! Here it is:

Author: Hui Xie
Email: [e-mail]

Generally how many patches one needs to do before they can apply for the commit access?

[ldionne]

In D116808#3244092, @huixie90 wrote:

Generally how many patches one needs to do before they can apply for the commit access?

Thanks, committed. I don't think there's any hard rule, but 4-5 patches is probably good, and it would be worth requesting access if you plan on contributing more to the project in the future.

(Also, side note but I wonder how/whether this is going to change if we more to GitHub PRs.)

libcxx/include/__ranges/concepts.h

namespace ranges {
   // `view_base` defined in <__ranges/enable_view.h>
 
   template <class _Tp>
   concept view =
     range<_Tp> &&
     movable<_Tp> &&
     enable_view<_Tp>;
 
-  template<class _Range>
+  template <class _Range>
   concept __simple_view =
     view<_Range> && range<const _Range> &&
     same_as<iterator_t<_Range>, iterator_t<const _Range>> &&
-    same_as<sentinel_t<_Range>, iterator_t<const _Range>>;
+    same_as<sentinel_t<_Range>, sentinel_t<const _Range>>;
 
   // [range.refinements], other range refinements
   template <class _Rp, class _Tp>
   concept output_range = range<_Rp> && output_iterator<iterator_t<_Rp>, _Tp>;
 
   template <class _Tp>
   concept input_range = range<_Tp> && input_iterator<iterator_t<_Tp>>;
 
   template <class _Tp>
   concept forward_range = input_range<_Tp> && forward_iterator<iterator_t<_Tp>>;

libcxx/test/libcxx/ranges/range.utility.helpers/simple_view.compile.pass.cpp

struct WrongConstView : std::ranges::view_base {
   const int *end() const;
 };
 
 struct NoConstView : std::ranges::view_base {
   int *begin();
   int *end();
 };
 
 struct DifferentSentinel : std::ranges::view_base {
   int *begin() const;
   sentinel_wrapper<int*> end() const;
 };
 
+struct WrongConstSentinel : std::ranges::view_base {
+  int *begin() const;
+  sentinel_wrapper<int*> end();
+  sentinel_wrapper<const int*> end() const;
+};
+
 static_assert( std::ranges::__simple_view<SimpleView>);
 static_assert(!std::ranges::__simple_view<WrongConstView>);
 static_assert(!std::ranges::__simple_view<NoConstView>);
-static_assert(!std::ranges::__simple_view<DifferentSentinel>);
+static_assert( std::ranges::__simple_view<DifferentSentinel>);
+static_assert(!std::ranges::__simple_view<WrongConstSentinel>);

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

 #include <ranges>
 #include <cassert>
 
 #include "test_macros.h"
 #include "test_iterators.h"
 #include "test_range.h"
 #include "types.h"
 
+namespace {
+
+struct NonCommonSimpleView : std::ranges::view_base {
+  int* begin() const;
+  sentinel_wrapper<int*> end() const;
+
+  // non const so that size_range<const DifferentSentinel> is false
+  size_t size();
+};
+
+} // namespace
+
 constexpr bool test() {
   int buffer[8] = {1, 2, 3, 4, 5, 6, 7, 8};
 
   // sized_range && random_access_iterator
   {
     std::ranges::take_view<SizedRandomAccessView> tv(SizedRandomAccessView(buffer), 4);
     assert(tv.begin() == SizedRandomAccessView(buffer).begin());
     ASSERT_SAME_TYPE(decltype(tv.begin()), RandomAccessIter);
constexpr bool test() {
   }
 
   {
     const std::ranges::take_view<MoveOnlyView> tv(MoveOnlyView{buffer}, 4);
     assert(tv.begin() == std::counted_iterator<int*>(buffer, 4));
     ASSERT_SAME_TYPE(decltype(tv.begin()), std::counted_iterator<int*>);
   }
 
+  // __simple_view<V> && sized_range<V> && !size_range<!V>
+  {
+    std::ranges::take_view<NonCommonSimpleView> tv(NonCommonSimpleView{}, 4);
+    ASSERT_SAME_TYPE(decltype(tv.begin()), std::counted_iterator<int*>);
+    // the optimal result should be int* but according to the c++20 standard,
+    // it returns std::counted_iterator<int*>
+  }
+
   return true;
 }
 
 int main(int, char**) {
   test();
   static_assert(test());
 
   return 0;
 }