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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const accepted this revision as: var-const.
var-const added a comment.

LGTM (with just a couple of minor comments). I'll leave final approval to @ldionne since he had comments as well.



================
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));
----------------
philnik wrote:
> var-const wrote:
> > I think this (an optimization, essentially) deserves a comment.
> I'm not sure what you mean? This is required by the standard.
We can still add a comment, even if this is essentially copied from the Standard. If I'm reading this correctly, this avoids wrapping a range in an `rvalue_view` if it's already a range of rvalues, to avoid unnecessary bloat. If that's correct, I think it deserves a comment.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.as.rvalue/ctor.pass.cpp:38
+template <class T, class... Args>
+concept IsImplicitlyConstructible = requires(T val, Args... args) { val = {std::forward<Args>(args)...}; };
+
----------------
Would `std::is_convertible` work?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137637