[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 marked 3 inline comments as done.
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)))
     {
----------------
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


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.all/all.pass.cpp:153
     static_assert(!std::is_invocable_v<decltype(std::views::all), RandomAccessRange, RandomAccessRange>);
+    static_assert(!std::is_invocable_v<decltype(std::views::all), MoveOnlyView&>);
   }
----------------
jloser wrote:
> var-const wrote:
> > Is it important for the view to be move-only? From the patch description, I got the impression that any reference should fail, but perhaps I'm missing something?
> The call operator already requires the template type be a view, so an lvalue reference to a view is the simplest case to make it go boom AFAICT.
"move-only" is important here because `_LIBCPP_AUTO_CAST` (which is used in the all's function body, and noexcept specification) is going to make a copy.

see definition here
```
#define _LIBCPP_AUTO_CAST(expr) static_cast<typename decay<decltype((expr))>::type>(expr)
```

If we pass an lvalue reference to a copyable `view`, the expression above is just making a copy of the `view`, which is fine. If the `view` is move-only, the above expression is ill-formed and cause `noexcept(decay-copy(v))` go boom


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128281