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

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

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

LGTM mod comments, all of which are nits, I think.



================
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
+  {
----------------
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>>);
```


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


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


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/types.h:112
+  int* end_;
+  constexpr explicit CommonView(int* b, int* e) noexcept : begin_(b), end_(e) { }
+  friend constexpr int* begin(CommonView& view) { return view.begin_; }
----------------
If this `noexcept` is needed, something is wonky. Ditto on line 124.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110433