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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/begin.pass.cpp:85-97
+  // simple_view<V> && random_access_range<const V> && sized_range<const V>
+  {
+    std::ranges::drop_view dropView(SimpleView{}, 4);
+    dropView.begin();
+    assert(!dropView.base().non_const_begin_called);
+  }
+
----------------
Quuxplusone wrote:
> ```
> {
>   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.
> 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);
}
```


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/begin.pass.cpp:102-106
+  {
+    using WithSimpleBase = std::ranges::join_view<SimpleParentView>;
+    static_assert(std::same_as<decltype(std::declval<WithSimpleBase&>().begin()),
+                               decltype(std::declval<WithSimpleBase const&>().begin())>);
+  }
----------------
Quuxplusone wrote:
> ```
> {
>   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)
> 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.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/begin.pass.cpp:76-77
+  {
+    using TakeNonSimpleView = std::ranges::take_view<NonCommonSimpleView>;
+    ASSERT_SAME_TYPE(decltype(std::declval<TakeNonSimpleView&>().begin()), std::counted_iterator<int*>);
+    // the optimal result should be int* but according to the c++20 standard,
----------------
Quuxplusone wrote:
> ```
> {
>   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*>);
> }
> ```
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116808