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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.nodiscard.verify.cpp:12
+// UNSUPPORTED: libcpp-has-no-incomplete-ranges
+// REQUIRES: libc++
+
----------------
jloser wrote:
> Question: since this is a `libc++` extension, should we move this file into `libcxx/test/libcxx` for example?
Yes, the convention would be to put those in `libcxx/test/libcxx`. However, I've been sort of on the fence about this recently, it feels easier and clearer to put those alongside the normal tests and just use `REQUIRES: libc++` to express that it's a libc++ specific test. The benefits I see are that you don't have to search for tests in two directories, and also there's no risk of the folder hierarchies getting out of sync if they get moved around.

I'll move it to `libcxx/test/libcxx` if you request it, otherwise I'll leave as-is.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.pass.cpp:32
+
+  // views::common(r) is equivalent to views::all(r) if r is a common_range
+  {
----------------
Quuxplusone wrote:
> It would be good to test a non-view type also (i.e. one where `all_t<R>` isn't `R`). Something like [UNTESTED]
> ```
> std::vector<int> v = {1, 2, 3};
> auto r = std::views::common(v);
> ASSERT_SAME_TYPE(decltype(r), std::ranges::ref_view<std::vector<int>>);
> ```
Good suggestion, done. I used `<array>` because it's `constexpr` friendly.


================
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)>);
+    }
----------------
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?


================
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)>);
+  }
----------------
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.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/types.h:116
+  friend constexpr int* end(CommonView& view) { return view.end_; }
+  friend constexpr int* end(CommonView const& view) { return view.end_; }
+};
----------------
Quuxplusone wrote:
> For my own information: Are the multiple overloads actually needed? I do have some vague recollection that they might be, for some poison-pill-related reason, but if so, geez, that's awful. :P
Yes, they are necessary for the poison pill thing, and yes, it's awful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110433