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

[Hui via Phabricator via libcxx-commits]

huixie90 requested changes to this revision.
huixie90 added a subscriber: jwakely.
huixie90 added a comment.
This revision now requires changes to proceed.

Thank you very much for working on this patch. The implementation looks pretty good to me.



================
Comment at: libcxx/include/__expected/expected.h:68
+
 namespace __expected {
 
----------------
philnik wrote:
> @huixie90 Why do we have this namespace? It doesn't really seem worth it just for `__throw_bad_expected_access`.
The honest answer is i can't remember. perhaps in some point of time I had several functions but got deleted in the end. But it seems that after this patch, there is more stuff in it.


================
Comment at: libcxx/include/__expected/expected.h:625
+    static_assert(is_convertible_v<_Up, _Err>, "argument has to be convertible to error_type");
+    return __has_val_ ? static_cast<_Err>(std::forward<_Up>(__v)) : __union_.__unex_;
+  }
----------------
philnik wrote:
> Let's use the public API here to match the wording. You can't `static_cast` here because that might call the wrong function: https://godbolt.org/z/TjPK9YWac. Please add a regression test. Same for the rvalue overload.
IIRC, Eric suggested to use `__has_val_` in place of `has_value()` (and other direct accessor member functions) to reduce the size in debug build


================
Comment at: libcxx/include/__expected/expected.h:94-105
       !is_reference_v<_Tp> &&
           !is_function_v<_Tp> &&
           !is_same_v<remove_cv_t<_Tp>, in_place_t> &&
           !is_same_v<remove_cv_t<_Tp>, unexpect_t> &&
           !__unexpected::__is_unexpected<remove_cv_t<_Tp>>::value &&
           __unexpected::__valid_unexpected<_Err>::value
       ,
----------------
This section is changed after this paper. Could you please update the `static_assert` and the message?
Please also add tests if there are behaviour changes.

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2505r5.html#orgb3c1202


================
Comment at: libcxx/include/__expected/expected.h:184
+  template <class _Func, class... _Args>
+  constexpr explicit expected(
+      __expected::__expected_construct_in_place_from_invoke_tag __tag, _Func&& __f, _Args&&... __args)
----------------
_LIBCPP_HIDE_FROM_ABI


================
Comment at: libcxx/include/__expected/expected.h:189
+  template <class _Func, class... _Args>
+  constexpr explicit expected(
+      __expected::__expected_construct_unexpected_from_invoke_tag __tag, _Func&& __f, _Args&&... __args)
----------------
_LIBCPP_HIDE_FROM_ABI


================
Comment at: libcxx/include/__expected/expected.h:621
 
+  template <class _Up = _Err>
+  _LIBCPP_HIDE_FROM_ABI constexpr _Err error_or(_Up&& __error) const& {
----------------
Could you please add test points for the default template argument?  (This applies to other overloads and `expected<void, E>::error_or`)
Without the default template parameter, this syntax would not work.
```
e.error_or({arg});
```

That being said, @jwakely why does `value_or` not have default template argument `class U = T` for consistency?


================
Comment at: libcxx/include/__expected/expected.h:819
+  _LIBCPP_HIDE_FROM_ABI constexpr auto transform_error(_Func&& __f) & {
+    using _Up = remove_cv_t<invoke_result_t<_Func, _Err&>>;
+    if (has_value()) {
----------------
It might be better not to call it `_Up` as `U` is usually paired with `T`. But here we are dealing with error type.
I would either stick to the spec to use `_Gp`, or choose some other names like `_OtherErr`. (same for other `transform_error` overloads


================
Comment at: libcxx/include/__expected/expected.h:1036
+  template <class _Func>
+  constexpr explicit expected(__expected::__expected_construct_in_place_from_invoke_tag, _Func&& __f)
+      : __has_val_(true) {
----------------
_LIBCPP_HIDE_FROM_ABI


================
Comment at: libcxx/include/__expected/expected.h:1042
+  template <class _Func, class... _Args>
+  constexpr explicit expected(
+      __expected::__expected_construct_unexpected_from_invoke_tag __tag, _Func&& __f, _Args&&... __args)
----------------
_LIBCPP_HIDE_FROM_ABI


================
Comment at: libcxx/include/__expected/expected.h:1214
 
+  template <class _Up = _Err>
+  _LIBCPP_HIDE_FROM_ABI constexpr _Err error_or(_Up&& __error) const& {
----------------
same for testing default argument and same comment for the name `_Up`


================
Comment at: libcxx/include/__expected/expected.h:1296-1299
+    static_assert(is_same_v<typename _Up::value_type, _Tp>,
+                  "The result of f(error()) must have the same value_type as this expected");
+    if (has_value()) {
+      return _Up();
----------------
optional: perhaps just use `void` instead of `_Tp` to improve the readability? I was slightly confused what `Up()` for a moment and later realised it was a `std::expected<void, E2>` so it can be default constructed


================
Comment at: libcxx/include/__expected/expected.h:1437
+    if (has_value()) {
+      return expected<_Tp, _Up>(in_place);
+    }
----------------
can we make it consistent with other 3 overloads?


================
Comment at: libcxx/test/std/utilities/expected/expected.expected/monadic/and_then.pass.cpp:17
+// template<class F> constexpr auto and_then(F&& f) const &&;
+
+#include <cassert>
----------------
could you please test 
> Constraints:  is_copy_constructible_v<E> is true.

Usually we have positive and negative `static_assert` to test whether or not the function exists

This applies to all functions with "Constraints"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140911