[libcxx-commits] [PATCH] D105753: [libcxx][ranges] Add `ranges::common_view`.

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__ranges/common_view.h:14-15
+#include <__iterator/common_iterator.h>
+#include <__ranges/view_interface.h>
+#include <__ranges/concepts.h>
+#include <concepts>
----------------
a-z plz (and throughout, if I missed any)


================
Comment at: libcxx/include/__ranges/common_view.h:84
+common_view(_Range&&)
+  -> common_view<decltype(views::all(_VSTD::declval<_Range>()))>;
+
----------------
Nit: lose the `_VSTD` on `declval`, for consistency with other libc++ headers. (It's ADL-proof by virtue of taking no arguments.)
But also, why is this not
```
template<class _Range>
common_view(_Range&&)
  -> common_view<views::all_t<_Range>>;
```
as in the standard's reference implementation? If we have `all`, we must have `all_t`, right?


================
Comment at: libcxx/include/ranges:119
 #include <__ranges/all.h>
+#include <__ranges/common_view.h>
 #include <__ranges/concepts.h>
----------------
Also update the modulemap.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/base.pass.cpp:28
+  constexpr ContiguousView& operator=(ContiguousView&&) = default;
+  constexpr friend int* begin(ContiguousView& view) { return view.ptr_; }
+  constexpr friend int* begin(ContiguousView const& view) { return view.ptr_; }
----------------
Style nit throughout (not a blocker): `friend constexpr`
https://quuxplusone.github.io/blog/2021/04/03/static-constexpr-whittling-knife/#friendness-to-befriend-an-entity


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/base.pass.cpp:72
+    std::ranges::common_view<ContiguousView> common(ContiguousView{buffer});
+    assert(std::move(common).base().ptr_ == buffer);
+  }
----------------
I suggest adding above line 72:
```
static_assert(!has_lvalue_qualified_base(common));
```
which could be implemented as
```
constexpr bool has_lvalue_qualified_base(auto&& view) {
    return requires { view.base(); };
}
```
This replaces your `BaseEnabled`, but puts the test in the salient place — right where we would expect to see the test for `common.base()`, you'd show the reason it's not tested. (This eliminates lines 59-63.)


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/begin.pass.cpp:80
+using RandomAccessIter = random_access_iterator<int*>;
+struct SizedRandomcAccessView : std::ranges::view_base {
+  int *ptr_;
----------------
/Randomc/Random/


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/begin.pass.cpp:93-98
+constexpr auto operator-(sentinel_wrapper<RandomAccessIter> sent, RandomAccessIter iter) {
+  return sent.base().base() - iter.base();
+}
+constexpr auto operator-(RandomAccessIter iter, sentinel_wrapper<RandomAccessIter> sent) {
+  return iter.base() - sent.base().base();
+}
----------------
I'd prefer to see these as hidden friends of `RandomAccessIter`, or know the technical reason why they can't be. Ditto lines 72-77 above.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/begin.pass.cpp:115
+    std::ranges::common_view<SizedForwardView> comm(SizedForwardView{buffer});
+    if (!std::is_constant_evaluated())
+      assert(*comm.begin() == 1);
----------------
Why do we need this `if`? What piece is not constexpr-friendly here?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/ctad.compile.pass.cpp:36
+
+struct BorrowableRange {
+  friend int* begin(BorrowableRange&);
----------------
I'd call this `BorrowedRange`. AIUI, the salient thing about a borrowed-range type is that it //is// borrowed, e.g. `string_view`. It's not merely borrow-able (like, I guess, `string` might be considered borrow-able).


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/ctad.compile.pass.cpp:46-55
+static_assert(std::same_as<decltype(std::ranges::common_view(View())), std::ranges::common_view<View>>);
+
+static_assert(std::same_as<decltype(std::ranges::common_view(std::declval<Range&>())),
+                           std::ranges::common_view<std::ranges::ref_view<Range>>>);
+
+static_assert(std::same_as<decltype(std::ranges::common_view(BorrowableRange())),
+                           std::ranges::common_view<
----------------
Consider named variables, and test both lvalues and rvalues.
```
void test_ctad() {
    View v;
    Range r;
    BorrowedRange br;
    static_assert(std::same_as<
        decltype(std::ranges::common_view(v)),
        std::ranges::common_view<View>
    >);
    static_assert(std::same_as<
        decltype(std::ranges::common_view(r)),
        std::ranges::common_view<std::ranges::ref_view<Range>>
    >);
    // std::ranges::common_view(std::move(r)) is ill-formed
    static_assert(std::same_as<
        decltype(std::ranges::common_view(br)),
        std::ranges::common_view<std::ranges::ref_view<BorrowedRange>>
    >);
    static_assert(std::same_as<
        decltype(std::ranges::common_view(std::move(br))),
        std::ranges::common_view<std::views::all_t<BorrowedRange>>
    >);
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105753