[libcxx-commits] [PATCH] D154116: [libc++] Implement LWG3938 (Cannot use std::expected monadic ops with move-only error_type)

[Yurong via Phabricator via libcxx-commits]

yronglin marked 2 inline comments as done.
yronglin added a comment.

Thanks for your review! @Mordante



================
Comment at: libcxx/docs/ReleaseNotes.rst:50
 - P2572R1 - ``std::format`` fill character allowances
+- LWG3938 - Cannot use ``std::expected`` monadic ops with move-only error_type
 
----------------
Mordante wrote:
> We typically don't mention LWG issues in our release notes, unless they have a huge impact.
Thanks for your tips, I'll remove that.


================
Comment at: libcxx/include/__expected/expected.h:694
   template <class _Func>
     requires is_constructible_v<_Tp, _Tp&>
   _LIBCPP_HIDE_FROM_ABI constexpr auto or_else(_Func&& __f) & {
----------------
Mordante wrote:
> Can you update this to the Standard wording too? the same for the other places.
I tried to keep the same with the std wording, but it compile failed:
```
error: invalid use of 'this' outside of a non-static member function
  694 |     requires is_constructible_v<_Tp, decltype(**this)>
      |                                                 ^
```


================
Comment at: libcxx/include/__expected/expected.h:694
   template <class _Func>
     requires is_constructible_v<_Tp, _Tp&>
   _LIBCPP_HIDE_FROM_ABI constexpr auto or_else(_Func&& __f) & {
----------------
yronglin wrote:
> Mordante wrote:
> > Can you update this to the Standard wording too? the same for the other places.
> I tried to keep the same with the std wording, but it compile failed:
> ```
> error: invalid use of 'this' outside of a non-static member function
>   694 |     requires is_constructible_v<_Tp, decltype(**this)>
>       |                                                 ^
> ```
As we talked in discord:

1. `*this` always produces an LValue

```
  template <class _Func>
    requires is_constructible_v<_Err, const _Err&&>
  _LIBCPP_HIDE_FROM_ABI constexpr auto and_then(_Func&& __f) const&& {
    static_assert(std::is_same_v<decltype(*this), const std::expected<_Tp, _Err>&&>);
    static_assert(std::is_same_v<decltype(**this), const _Tp&&>);
    using _Up = remove_cvref_t<invoke_result_t<_Func, decltype(**this)>>;
    static_assert(
        __is_std_expected<_Up>::value, "The result of f(std::move(value())) must be a specialization of std::expected");
    static_assert(is_same_v<typename _Up::error_type, _Err>,
                  "The result of f(std::move(value())) must have the same error_type as this expected");
    if (has_value()) {
      return std::invoke(std::forward<_Func>(__f), std::move(**this));
    }
    return _Up(unexpect, std::move(error()));
  }
```
the two `static_assert` fails:
```
 static_assert(std::is_same_v<decltype(*this), const std::expected<_Tp, _Err>&&>);
 static_assert(std::is_same_v<decltype(**this), const _Tp&&>);
```
when `and_then` called by:
```
#include <expected>
#include <cassert>

struct UnexpectedCRVal {
  std::expected<int, int> operator()(int&)       = delete;
  std::expected<int, int> operator()(const int&) = delete;
  std::expected<int, int> operator()(int&&)      = delete;
  constexpr std::expected<int, int> operator()(const int&&) { return std::expected<int, int>(std::unexpected<int>(5)); }
};

int main() {
    const std::expected<int, int> e{0};
    assert(std::move(e).and_then(UnexpectedCRVal{}).error() == 5);
    return 0;
}
```

The reason is that: `*this` always produces an Lvalue http://eel.is/c++draft/expr.unary.op#1.sentence-3 , so the type of `decltype(**this)` is `const _Tp &` but not `const _Tp &&` 

2. we can use no-static member function in constraints

std wording: `Constraints: is_constructible_v<E, decltype(std​::​move(error()))> is true.`
we can't use `decltype(std​::​move(error()))` outside a non-static member function, and we use `const _Err&&` as a workaround before.
Also `is_constructible_v<decltype(**this)>` in constraints has the same issue:
```
error: invalid use of 'this' outside of a non-static member function
  694 |     requires is_constructible_v<_Tp, decltype(**this)>
          |      
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154116