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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik marked an inline comment as done.
philnik added inline comments.


================
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));
----------------
var-const wrote:
> 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.
AFAIK lots of views do this, and we don't comment anywhere else. Adding a comment here makes it look like this is an extension.


================
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)...}; };
+
----------------
var-const wrote:
> Would `std::is_convertible` work?
Not for the default constructible case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137637