[libcxx-commits] [PATCH] D103493: [libcxx][ranges] Add concepts in range.utility.helpers.

Arthur O'Dwyer via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Wed Jun 2 10:04:09 PDT 2021


Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added inline comments.


================
Comment at: libcxx/test/libcxx/ranges/range.utility.helpers/simple_view.compile.pass.cpp:29-32
+struct NoConstView : std::ranges::view_base {
+  friend int* begin(NoConstView&);
+  friend int* end(NoConstView&);
+};
----------------
zoecarver wrote:
> Quuxplusone wrote:
> > zoecarver wrote:
> > > Quuxplusone wrote:
> > > > I suggest you add a positive example `OnlyConstView`, with only `friend const int* begin(const OnlyConstView&)`.
> > > > And then you can eliminate lines 40 and 42 from `DifferentSentinel`.
> > > This isn't a view. A view requires that T is movable (i.e., not const) and that we can do `ranges::begin(declval<T&>())`. Therefor, we need all four members.
> > @zoecarver: I believe you've misunderstood. I'm asking that you add this test case:
> > ```
> > struct OnlyConstView : std::ranges::view_base {
> >   friend int* begin(const OnlyConstView&);
> >   friend int* end(const OnlyConstView&);
> > };
> > static_assert(std::ranges::__simple_view<OnlyConstView>);
> > ```
> > And then modify `DifferentSentinel` as follows:
> > ```
> > struct DifferentSentinel : std::ranges::view_base {
> >   friend int* begin(DifferentSentinel const&);
> >   friend sentinel_wrapper<int*> end(DifferentSentinel const&);
> > };
> > static_assert(!std::ranges::__simple_view<DifferentSentinel>);
> > ```
> No, I understood you. Maybe the following snippet will help me explain what I'm trying to say:
> ```
> struct OnlyConstView : std::ranges::view_base {
>   friend int* begin(const OnlyConstView&);
>   friend int* end(const OnlyConstView&);
> };
> static_assert(!std::ranges::range<OnlyConstView>);
> static_assert( std::ranges::range<const OnlyConstView>);
> static_assert(!std::ranges::view<OnlyConstView>);
> static_assert(!std::ranges::view<const OnlyConstView>);
> static_assert(!std::ranges::__simple_view<OnlyConstView>);
> static_assert(!std::ranges::__simple_view<const OnlyConstView>);
> ```
> 
> Here, you can see that only `const OnlyConstView` is a `range`. This is because a range requires us to be able to do `ranges::begin(declval<T&>())` (so `T` must be const). You'll also notice that in no case is this a view, because a view requires `T` to be movable, so it cannot be const. And `__simple_view` requires `T` to be a view, so this ins't a `__simple_view` either. 
Ahhh. This is https://cplusplus.github.io/LWG/issue3480 — the poison pill for `ranges::begin` is "bad" in the sense that `begin(auto&) // poison-pill` is a better-matching candidate than our actual ADL `begin(const OnlyConstView&)`.
So the solution adopted in LWG3480 is to pass views by value, not by const reference — and arguably that's a more "classic STL" way to do it, anyway.
I'd replace this with:
```
struct ValueView : std::ranges::view_base {
  friend int* begin(ValueView);
  friend int* end(ValueView);
};
static_assert( std::ranges::range<ValueView>);
static_assert( std::ranges::range<const ValueView>);
static_assert( std::ranges::view<ValueView>);
static_assert(!std::ranges::view<const ValueView>);  // because const types are not assignable-to
static_assert( std::ranges::__simple_view<ValueView>);
```
However, if you think this is too far from the original intent of the test, I guess I'm ambivalent at this point. It basically depends on whether we think "pass views by value" is a good general rule.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103493



More information about the libcxx-commits mailing list