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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added inline comments.
This revision is now accepted and ready to land.


================
Comment at: libcxx/include/__iterator/move_sentinel.h:53
 
+_LIBCPP_CTAD_SUPPORTED_FOR_TYPE(move_sentinel);
+
----------------
ldionne wrote:
> Can you please add a simple test that exercises this ctad, even if it is the default-provided one by the compiler?
I don't think this was addressed. Please try to ensure that you've addressed feedback before marking a comment as done, otherwise it's really easy to drop (potentially important) comments on the floor. Reviewers usually assume that the feedback has been implemented correctly when the comment is marked as done.


================
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:
> > 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.
I would agree that no comment is needed since this is indeed what the standard specifies. I'm actually not sure what the comment would say.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137637