[libcxx-commits] [PATCH] D116991: [libc++] [ranges] SFINAE away ranges::cbegin(const T&&) for non-borrowed T.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__ranges/access.h:166
     template <class _Tp>
+      requires is_lvalue_reference_v<_Tp&&>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
----------------
CaseyCarter wrote:
> `&&` is extraneous - `_Tp` is an lvalue reference if and only if `_Tp&&` is an lvalue reference. (Also on 194.)
Yes, but I think this will be easier on the reader. The standard says "If //`E`// is an lvalue...", and here the type of //`E`// is `_Tp&&`, so, I say let's just stick with that instead of being any more "clever."

Notice that a "clever" usage of `_Tp` instead of `_Tp&&` was the root cause of https://github.com/llvm/llvm-project/issues/52953 .


================
Comment at: libcxx/include/__ranges/access.h:171
+      -> decltype(      ranges::begin(static_cast<const __uncvref_t<_Tp>&>(__t)))
+      { return          ranges::begin(static_cast<const __uncvref_t<_Tp>&>(__t)); }
 
----------------
CaseyCarter wrote:
> I'd use `remove_reference_t` instead of `__uncvref_t` to preserve `volatile`. (6 places, including lines 197-199)
Hm, I guess that depends on the interpretation of https://eel.is/c++draft/range.access.cbegin . We're supposed to take a "subexpression `E` of type `T`" and cast it to `const T&`. Suppose the expression is of type `U=int&` — then `const U&` would be `int&`, but in fact we want to say that `T=int`. So "of type `T`" strips ref-qualification; does it also strip cv-qualification? Is a reference to a const int an expression "of type `int`" or "of type `const int`"? Is a reference to a volatile int an expression "of type `int`" or "of type `volatile int`"?

I originally only picked `__uncvref_t` over `remove_reference_t` because it was shorter to type. I'll change it, although I wonder if we should test this `volatile` business, or ask LWG the intent, or what.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116991