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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

@var-const Can you make a pass at this?



================
Comment at: libcxx/include/__iterator/move_sentinel.h:53
 
+_LIBCPP_CTAD_SUPPORTED_FOR_TYPE(move_sentinel);
+
----------------
Can you please add a simple test that exercises this ctad, even if it is the default-provided one by the compiler?


================
Comment at: libcxx/include/__ranges/as_rvalue_view.h:61
+  _LIBCPP_HIDE_FROM_ABI constexpr auto end()
+    requires(!__simple_view<_View>)
+  {
----------------
Do you have tests for a simple view and a non-simple view?


================
Comment at: libcxx/include/__ranges/as_rvalue_view.h:74
+    if constexpr (common_range<const _View>) {
+      return move_iterator(ranges::end(__base_));
+    } else {
----------------
Do you have tests for a common range and a non-common range?


================
Comment at: libcxx/include/__ranges/as_rvalue_view.h:94
+template <class _Range>
+as_rvalue_view(_Range&&) -> as_rvalue_view<views::all_t<_Range>>;
+
----------------
Do you have a test that would fail if you dropped the `all_t` here?


================
Comment at: libcxx/include/__ranges/as_rvalue_view.h:103
+  template <class _Range>
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Range&& __range) const
+      noexcept(noexcept(/**/ as_rvalue_view(std::forward<_Range>(__range))))
----------------
This `[[nodiscard]]` is an extension, so it should be using `_LIBCPP_NODISCARD_EXT` unless I am mistaken. Please make sure we have a libc++ specific test for this extension.

We might be doing it wrong for other adaptors, in which case we should also fix them, but not in this patch.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.as.rvalue/end.pass.cpp:90
+  test_range<cpp20_input_iterator<int*>, sized_sentinel<cpp20_input_iterator<int*>>, false>();
+  test_range_sentinels<forward_iterator<int*>>();
+  test_range_sentinels<bidirectional_iterator<int*>>();
----------------
Should we be using the new facilities with `meta::for_each` here? (everywhere)


================
Comment at: libcxx/test/support/test_iterators.h:994
+
+  rvalue_iterator& operator++() {
+    ++it_;
----------------
Any reason why this is not constexpr when it can be? (everywhere)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137637