[libcxx-commits] [PATCH] D140911: [libc++] Implement P2505R5(Monadic operations for std::expected).

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D140911#4326750 <https://reviews.llvm.org/D140911#4326750>, @yronglin wrote:

> In D140911#4325192 <https://reviews.llvm.org/D140911#4325192>, @Mordante wrote:
>
>> Sorry sometimes patches get a bit lost in the review queue.
>> There are some things @ldionne should look at, I've asked him to have a look.
>
> Many thanks!

Thanks for the ping on Discord Mark. @yronglin Sorry for not seeing your pings, notifications on Phab don't allow subscribing to direct mentions so I don't see them, they get lost in other review traffic. Next time, don't hesitate to ping me on Discord (yeah I know it's a pain).

This almost LGTM but I have a question about constraints.



================
Comment at: libcxx/include/__expected/expected.h:641
+  template <class _Func>
+    requires is_copy_constructible_v<_Err>
+  _LIBCPP_HIDE_FROM_ABI constexpr auto and_then(_Func&& __f) & {
----------------
I don't quite understand why https://eel.is/c++draft/expected#object.monadic-2 says `is_constructible_v<E, decltype(error())>` but we're using `is_copy_constructible_v<_Err>`. I don't think those are equivalent, right? For example `error()` will return a non-const reference for this `&`-qualified function, but we're checking that `is_constructible_v<E, E const&>` instead of `is_constructible_v<E, E&>`? This is subtle, but if I'm right, we should add a test for this and then correct all the overloads below.


================
Comment at: libcxx/include/__expected/expected.h:644-645
+    using _Up = remove_cvref_t<invoke_result_t<_Func, _Tp&>>;
+    static_assert(
+        std::__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>,
----------------
Here and below, qualification for this isn't required. We qualify function calls to avoid ADL, but this is not a function call.


================
Comment at: libcxx/test/std/utilities/expected/expected.expected/monadic/transform.pass.cpp:12
+// GCC has a issue for `Guaranteed copy elision for potentially-overlapping non-static data members`,
+// please refer to: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108333 , But we have a workaround to
+// avoid this issue.
----------------



================
Comment at: libcxx/test/std/utilities/expected/expected.expected/monadic/transform_error.pass.cpp:12
+// GCC has a issue for `Guaranteed copy elision for potentially-overlapping non-static data members`,
+// please refer to: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108333 , But we have a workaround to
+// avoid this issue.
----------------



================
Comment at: libcxx/test/std/utilities/expected/expected.void/monadic/transform_error.pass.cpp:12
+// GCC has a issue for `Guaranteed copy elision for potentially-overlapping non-static data members`,
+// please refer to: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108333 , But we have a workaround to
+// avoid this issue.
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140911