[libcxx-commits] [PATCH] D128281: [libc++] fix views::all hard error on lvalue move only views instead of SFINAE

[Hui via Phabricator via libcxx-commits]

huixie90 added inline comments.


================
Comment at: libcxx/include/__ranges/all.h:42
       noexcept(noexcept(_LIBCPP_AUTO_CAST(std::forward<_Tp>(__t))))
+      -> decltype(_LIBCPP_AUTO_CAST(std::forward<_Tp>(__t)))
     {
----------------
ldionne wrote:
> huixie90 wrote:
> > EricWF wrote:
> > > huixie90 wrote:
> > > > EricWF wrote:
> > > > > var-const wrote:
> > > > > > I wonder if we have any other cases where we don't write the return type 3 times and consequently don't get SFINAE.
> > > > > Is this a case of "not getting SFINAE" or "auto" return types not deducing to reference types? Does this work if we use `decltype(auto)` instead?
> > > > > 
> > > > > Also this changes the return type from being by-value to being by-reference, which is the intended change I assume?
> > > > This function always return by value because `_LIBCPP_AUTO_CAST` always yield the decayed type
> > > > 
> > > > ```
> > > > #define _LIBCPP_AUTO_CAST(expr) static_cast<typename decay<decltype((expr))>::type>(expr)
> > > > ```
> > > > 
> > > > In the spec, it says expression equivalent to `decay-copy(E)`, and  `_LIBCPP_AUTO_CAST` is the libc++ way of implementing `decay-copy`
> > > > 
> > > > In any case, the return type should be the same as `decay-copy` (expression equivalent), and this is why `decltype(_LIBCPP_AUTO_CAST(...))` is correct
> > > > 
> > > > `decltype(auto)` won't work because `decltype(auto)` doesn't have any effect on SFINAE
> > > Oh, based on the name I had assumed it did something entirely different. Like forward<T&&>(t), but without the need to write the type.
> > > Maybe it needs a better name.
> > > 
> > > Also, this is a lesson in using `auto` return types. I think it's clear that:
> > > 
> > > 1. We shouldn't use them in any place the standard doesn't mandate.
> > > 1.  We really shouldn't use them in any place we also use `noexcept(noexcept(...))`, because it can poison an entire overload set as your discovering.
> > yeah, i was expecting a `__decay_copy`  function but it turned out to have this macro. I guess the name was inspired by the C++23 `auto(x)` syntax which is almost the same as this macro except when `x` is a prvalue, this macro will  do an  extra move.
> @huixie90 Yes, `_LIBCPP_AUTO_CAST` was inspired by `auto(x)`. In fact, we used to have a `__decay_copy` function and we removed it in favour of `_LIBCPP_AUTO_CAST` in  D115686.
Thanks. correction for my previous comment. `__decay_copy` will do an extra move for prvalue inputs,  `auto(x)` and `_LIBCPP_AUTO_CAST` will not do the extra move


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128281