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

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a subscriber: Mordante.
Quuxplusone added a comment.

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.



================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/begin.pass.cpp:42
+  constexpr int* end() const { return nullptr; }
+  constexpr std::size_t size() const { return 0u; }
+};
----------------



================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/begin.pass.cpp:90
+    LIBCPP_STATIC_ASSERT(std::ranges::__simple_view<SimpleView>);
+    int non_const_calls = 0, const_calls = 0;
+    std::ranges::drop_view dropView(SimpleView{{}, &non_const_calls, &const_calls}, 4);
----------------



================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/begin.pass.cpp:99-100
+  {
+    static_assert(std::ranges::random_access_range<const NonSimpleView> &&
+                  std::ranges::sized_range<const NonSimpleView>);
+    LIBCPP_STATIC_ASSERT(!std::ranges::__simple_view<NonSimpleView>);
----------------



================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/begin.pass.cpp:103
+    int non_const_calls = 0, const_calls = 0;
+    std::ranges::drop_view dropView(NonSimpleView{{}, &non_const_calls, &const_calls}, 4);
+    assert(dropView.begin() == nullptr);
----------------
(Aside: That extra `{}` is an initializer for the empty base class `view_base`? Yuck. But if the language requires it... okay.) 


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/begin.pass.cpp:104
+    std::ranges::drop_view dropView(NonSimpleView{{}, &non_const_calls, &const_calls}, 4);
+    assert(dropView.begin() == nullptr);
+    assert(std::as_const(dropView).begin() == nullptr);
----------------
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`.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/begin.pass.cpp:29-30
+struct SimpleParentView : std::ranges::view_base {
+  const ChildView* begin() const { return nullptr; }
+  const ChildView* end() const { return nullptr; }
+};
----------------
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!)


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/begin.pass.cpp:109
+  {
+    std::ranges::join_view<SimpleParentView> jv{};
+    static_assert(std::same_as<decltype(jv.begin()), 
----------------
Here and line 102: remove the `{}`.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/begin.pass.cpp:24-30
+struct NonCommonSimpleView : std::ranges::view_base {
+  int* begin() const { return nullptr; }
+  sentinel_wrapper<int*> end() const { return sentinel_wrapper<int*>{nullptr}; }
+
+  // non const so that size_range<const DifferentSentinel> is false
+  size_t size() { return 0u; }
+};
----------------
```
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.)


================
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,
----------------
huixie90 wrote:
> 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 
On line 76, please remove the `{}`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116808