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

[Yurong via Phabricator via libcxx-commits]

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

Thanks for your comments @huixie90 !



================
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
       ,
----------------
huixie90 wrote:
> 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
+1


================
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)
----------------
huixie90 wrote:
> _LIBCPP_HIDE_FROM_ABI
+1


================
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)
----------------
huixie90 wrote:
> _LIBCPP_HIDE_FROM_ABI
+1


================
Comment at: libcxx/include/__expected/expected.h:621
 
+  template <class _Up = _Err>
+  _LIBCPP_HIDE_FROM_ABI constexpr _Err error_or(_Up&& __error) const& {
----------------
huixie90 wrote:
> 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?
+1


================
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()) {
----------------
huixie90 wrote:
> 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
+1, _Up -> _Gp


================
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) {
----------------
huixie90 wrote:
> _LIBCPP_HIDE_FROM_ABI
+1


================
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)
----------------
huixie90 wrote:
> _LIBCPP_HIDE_FROM_ABI
+1


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


================
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();
----------------
huixie90 wrote:
> 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
```
return expected<_Tp, _Gp::error_type>();
```
or
```
return expected<void, _Gp::error_type>(); 
```
What do you think about? But it's a little bit not closer to the wording


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140911