[libcxx-commits] [PATCH] D116199: [libc++] Fix ranges::{cbegin, cend} for rvalues.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__ranges/access.h:160
 namespace ranges {
 namespace __cbegin {
   struct __fn {
----------------
I actually don't understand why we specify our implementation this way at all. If we look at http://eel.is/c++draft/range.access#cbegin, I would expect this instead:

```
struct __fn {
  template <class _Tp>
    requires is_lvalue_reference_v<_Tp>
  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __t) const
    noexcept(noexcept(ranges::begin(static_cast<_Tp const&>(__t)))
    -> decltype(ranges::begin(static_cast<_Tp const&>(__t)))
    { return ranges::begin(static_cast<_Tp const&>(__t)); }

  template <class _Tp>
  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __t) const
    noexcept(noexcept(ranges::begin(static_cast<_Tp const&&>(__t)))
    -> decltype(ranges::begin(static_cast<_Tp const&&>(__t)))
    { return ranges::begin(static_cast<_Tp const&&>(__t)); }
};
```

[untested but you get the point]


Edit: It looks like you are doing that as part of D115607, and I think it would be fine to pull that part of the change into this review. You could rename it to something like "implement `ranges::cbegin`/`ranges::cend` per the spec, and you're adding a test for the one thing you know is being fixed concretely (in addition to making us closer to the spec).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116199