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

[Casey Carter via Phabricator via libcxx-commits]

CaseyCarter 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
----------------
Quuxplusone wrote:
> 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 .
There are no expressions of reference type. The type of `E` is `remove_reference_t<_Tp>`. `E` is an lvalue if `is_lvalue_reference_v<_Tp>` and an rvalue otherwise. We can't determine if an rvalue `E` is a prvalue or an xvalue, because we bound a reference to it forcing materialization of prvalues into xvalues. The argument to this function is in any case a glvalue, which denotes the object the Standard refers to as "the reified object of `E`".

That said, the suggested change is less important to me than being pedantically correct in our discussion of the change =)


================
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)); }
 
----------------
Quuxplusone wrote:
> 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.
> 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?

Again, there are no expressions of reference type - it's important that we don't conflate the type of an expression with the type-and-value-category-encoding `decltype((meow))`. When `decltype((E))` is `int&`, `E` is an lvalue of type `int` - `const T&` is `const int&`. 

> Is a reference to a const int an expression "of type `int`" or "of type `const int`"?

Super-pedantically, a reference isn't an expression anymore than an array or a class is an expression. An id-expression that names a variable of type `const int&` (or a function-call expression that invokes a function whose return type is `const int&`, or ...) is an expression, specifically an lvalue of type `const int`. 

> Is a reference to a volatile int an expression "of type `int`" or "of type `volatile int`"?

Ditto-ish. In general, when `E` is an expression, `remove_reference_t<decltype((E))>` is its type, and it is an lvalue if `is_lvalue_reference_v<decltype((E))>`, an xvalue if `is_rvalue_reference_v<decltype((E))>`, or a prvalue if `!is_reference_v<decltype((E))>`.
 
> 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.

In my experience, LWG mostly wants to pretend that volatile doesn't exist. That said, it's as easy to preserve the `volatile` here as not, and I'd prefer errors to be at the call to `ranges::begin` instead of inside.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116991