[libcxx-commits] [PATCH] D137637: [libc++] Implement P2446R2 (views::as_rvalue)

Fri Dec 2 15:03:02 PST 2022

var-const added inline comments.

================
Comment at: libcxx/include/__ranges/as_rvalue_view.h:3
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
----------------
Please update the `<ranges>` synopsis as well.

================
Comment at: libcxx/include/__ranges/as_rvalue_view.h:30
+  requires input_range<_View>
+class as_rvalue_view : public view_interface<as_rvalue_view<_View>> {
+  _LIBCPP_NO_UNIQUE_ADDRESS _View __base_ = _View();
----------------
This needs an include (probably currently relying on the transitive include from `all.h`).

================
Comment at: libcxx/include/__ranges/as_rvalue_view.h:110-114
+    requires same_as<range_rvalue_reference_t<_Range>, range_reference_t<_Range>>
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Range&& __range) const
+      noexcept(noexcept(/**/ views::all(std::forward<_Range>(__range))))
+          -> decltype(/*--*/ views::all(std::forward<_Range>(__range))) {
+    return /*-------------*/ views::all(std::forward<_Range>(__range));
----------------
I think this (an optimization, essentially) deserves a comment.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.as.rvalue/adaptor.pass.cpp:24
+
+static_assert(std::random_access_iterator<rvalue_iterator<int*>>);
+
----------------
I think this check would be better in `test_iterators.h`, right after the definition of `rvalue_iterator`.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.as.rvalue/adaptor.pass.cpp:44
+constexpr bool test() {
+  { // view | views::as_rvalue
+    View v{{}, 3};
----------------
Can you also check piping a container (i.e. something that is not a view) through `as_rvalue`? Ideally, both lvalue and rvalue.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.as.rvalue/adaptor.pass.cpp:48
+    assert(view.base().i == 3);
+  }
+  { // adaptor | views::as_rvalue
----------------
Nit: can you add empty lines between the cases? (Here and in some other tests)

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.as.rvalue/adaptor.pass.cpp:52
+    const auto partial = std::views::transform(std::identity{}) | std::views::as_rvalue;
+    std::same_as<std::ranges::as_rvalue_view<std::ranges::transform_view<View, std::identity>>> auto view = partial(v);
+    assert(view.base().base().i == 3);
----------------
`s/auto/decltype(auto)/` (throughout).

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.as.rvalue/adaptor.pass.cpp:55
+  }
+  { // adaptor | views::as_rvalue
+    View v{{}, 3};
----------------
The argument order is swapped in the comment.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.as.rvalue/adaptor.pass.cpp:73
+  test();
+  static_assert(test());
+}
----------------
Nit: `return 0;`.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.as.rvalue/begin.pass.cpp:29
+static_assert(HasBegin<std::ranges::as_rvalue_view<View>>);
+static_assert(HasBegin<const std::ranges::as_rvalue_view<View>>);
+
----------------
These checks don't seem to be exhaustive. For example, there should probably be a check that `!HasBegin` if `!range<const _View>`.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.as.rvalue/begin.pass.cpp:32
+template <class Iter, class Sent>
+constexpr void test_range() {
+  int a[] = {1, 2};
----------------
This only tests the non-const version of `begin`, right? I think it shouldn't be too hard to also test a const `as_rvalue_view` here.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.as.rvalue/begin.pass.cpp:42
+constexpr void test_range_sentinels() {
+  test_range<Iter, Iter>();
+  test_range<Iter, sentinel_wrapper<Iter>>();
----------------
Optional: I think you could wrap this call in a check like `if constexpr (std::sentinel_for<Iter, Iter>)` and avoid having to call `test_range` directly from `test`.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.as.rvalue/end.pass.cpp:58
+template <class T>
+concept HasBegin = requires(T t) { t.end(); };
+
----------------
`s/Begin/End/`.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.as.rvalue/end.pass.cpp:61
+static_assert(HasBegin<std::ranges::as_rvalue_view<View>>);
+static_assert(HasBegin<const std::ranges::as_rvalue_view<View>>);
+
----------------
Can you also test the case where the concept is false?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.as.rvalue/end.pass.cpp:70
+constexpr void test_range() {
+  using Expected = std::conditional_t<is_common, std::move_iterator<Sent>, std::move_sentinel<Sent>>;
+  int a[]        = {1, 2};
----------------
Is it possible to just use the `std::ranges::common_range` concept?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.as.rvalue/end.pass.cpp:73
+  std::ranges::subrange range(Iter(std::begin(a)), Sent(Iter(std::end(a))));
+  std::ranges::as_rvalue_view view(std::move(range));
+  std::same_as<Expected> decltype(auto) iter = view.end();
----------------
Same as the `begin` tests -- I think this should also test when a view is `const`.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.as.rvalue/end.pass.cpp:80
+constexpr void test_range_sentinels() {
+  test_range<Iter, Iter, true>();
+  test_range<Iter, sentinel_wrapper<Iter>, false>();
----------------
Same as the `begin` test -- I think this can be simplified with `if constexpr`.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.as.rvalue/end.pass.cpp:98
+    std::ranges::as_rvalue_view view(CVCallView{});
+    (void)view.begin();
+    assert(view.base().const_called);
----------------
Why are we checking `begin` here?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.as.rvalue/size.pass.cpp:53
+  {
+    bool size_called = false;
+    std::ranges::as_rvalue_view view(ConstSizedView{{}, &size_called});
----------------
Doesn't seem to be checked. Also, why is it only on the const sized view and not the non-const one?

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.as.rvalue/size.pass.cpp:56
+    std::same_as<size_t> auto size = view.size();
+    assert(size == 3);
+  }
----------------
Why is the return value only checked for the const case?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137637