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

[Hui via Phabricator via libcxx-commits]

huixie90 added a comment.

@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)



================
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:
> 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);
> }
> ```
that sounds good . i will update then


================
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:
> 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.
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


================
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:
> 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.
sounds good 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116808