[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 4 inline comments as done.
yronglin added a comment.

In D154116#4612494 <https://reviews.llvm.org/D154116#4612494>, @frederick-vs-ja wrote:

> In D154116#4490038 <https://reviews.llvm.org/D154116#4490038>, @yronglin wrote:
>
>> @Mordante Can we propose to use `__union_.__val_`, `std::as_const(__union_.__val_)`, `std::move(__union_.__val_)` and `std::move(std::as_const(__union_.__val_))` instead of `**this` ?
>
> `std::as_const` is unnecessary because the `const` on the member functions already makes `__union_.__val_` a const lvalue.
>
> I think it's better to use the member access expressions instead of `**this`, because the latter can be ADL-hijacked. (However, this means the current standard wording is less than ideal and an LWG issue may be needed, see also LWG3969 <https://cplusplus.github.io/LWG/issue3969>).

Thanks a lot for your review! I updated to use `__union_.__val_`. and also update the static_assert message. @Mordante WDYT?



================
Comment at: libcxx/include/__expected/expected.h:643
     using _Up = remove_cvref_t<invoke_result_t<_Func, _Tp&>>;
     static_assert(__is_std_expected<_Up>::value, "The result of f(value()) must be a specialization of std::expected");
     static_assert(is_same_v<typename _Up::error_type, _Err>,
----------------
frederick-vs-ja wrote:
> It might be better to update the messages in `static_assert` (also occurs below; I originally suggested [[ https://github.com/cplusplus/draft/issues/6500#issuecomment-1689123585 | here ]].).
fixed


================
Comment at: libcxx/test/std/utilities/expected/expected.expected/monadic/and_then.pass.cpp:7
 //
 //===----------------------------------------------------------------------===//
 
----------------
Mordante wrote:
> Please update the `or_else` and `transform` test too.
fixed.


================
Comment at: libcxx/test/std/utilities/expected/expected.expected/monadic/and_then.pass.cpp:142
+  NonCopyable(const NonCopyable&&) {}
 };
 
----------------
Mordante wrote:
> Can you make a separate MoveOnly class instead.
I have added a new `MoveOnlyErrorType` class


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154116