[libcxx-commits] [PATCH] D110433: [libc++] Add the std::views::common range adaptor

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

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.pass.cpp:88
+      static_assert( CanBePiped<SomeView&, decltype(std::views::common)>);
+      static_assert(!CanBePiped<NotAView,  decltype(std::views::common)>);
+    }
----------------
ldionne wrote:
> Quuxplusone wrote:
> > Also
> > ```
> > static_assert(!CanBePiped<std::vector<int>, decltype(std::views::common)>);
> > ```
> > or perhaps replace line 88 with
> > ```
> > static_assert( CanBePiped<int(&)[10], decltype(std::views::common)>);
> > static_assert(!CanBePiped<int(&&)[10], decltype(std::views::common)>);
> > static_assert(!CanBePiped<int, decltype(std::views::common)>);
> > ```
> I'm curious to understand why `int(&&)[10]` can't be piped?
Assuming I'm correct that `int(&&)[10]` can't be piped :) my intuition is that rvalues of non-borrowed-range types can never be piped. `int(&&)[10]` is not an lvalue reference type, and `int[10]` is not a borrowed-range type; Q.E.D.

We aren't used to thinking about array rvalues in C++, but they're just like any other rvalue. See https://quuxplusone.github.io/blog/2021/08/07/p2266-field-test-results/#libreoffice-ostring for another example from like a month ago.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.pass.cpp:93
+  {
+    static_assert(std::same_as<decltype(std::views::common), decltype(std::ranges::views::common)>);
+  }
----------------
ldionne wrote:
> Quuxplusone wrote:
> > Not only that, but `std::addressof(std::views::common) == std::addressof(std::ranges::views::common)`.
> > I don't know if we really care about testing this; but if we do, then perhaps we should test //all// the members of namespace `views`, in one single test file, together.
> We're not supposed to be able to take the address of those so I think it's reasonable not to test for that.
I agree; but I meant that right now you're just testing that `views::x` and `ranges::views::x` have the same //type//; you're not actually testing that they are //two different names for the same object// (which is what we want).
You can definitely pass `std::views::common` by reference to a function — that is a deliberate supported use-case — so you can get its address by using `addressof` on that reference. I don't know where we draw the line between "manipulating an object by reference such that you obtain a pointer to the object" (which is fine) and "taking the object's address" (which is allegedly forbidden). ;)
Anyway, minor point. I still think that if we're spending brain cells on this, we should do it for //all// the views, in one place, not scattered. And if we're not spending brain cells on it, then ship it. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110433