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

Wed Jan 4 08:02:27 PST 2023

philnik added inline comments.

================
Comment at: libcxx/include/__expected/expected.h:75
   (void)__arg;
-  std::abort();
+  _VSTD::abort();
 #  endif
----------------
yronglin wrote:
> huixie90 wrote:
> > why all `std`s are changed to `_VSTD`? I thought we are always using `std` now
> I have refer to the comments in https://reviews.llvm.org/D113408 , 
> ```
> Note that this qualification of _VSTD::move is good because it protects against ADL. My comment above about qualifications do not apply to cases that prevent ADL from being triggered.
> ```
> So, I understand it is better to use _VSTD? I don't know if I got it wrong, if so I would undo these changes.
Qualifying the calls fixes this, i.e. calling `abort()` is an ADL call, but `std::abort()` isn't. `_VSTD` just expands to `std`, there is no difference other than readability.

================
Comment at: libcxx/include/__expected/expected.h:867-871
+   union {
+     __empty_t __empty_;
+     _Tp __val_;
+     _Err __unex_;
+  };
----------------
yronglin wrote:
> huixie90 wrote:
> > I am not sure I understand this change. is this due to a wrong rebase or something? the purpose of `__empty_t` and the "named" union was to be able to apply `_LIBCPP_NO_UNIQUE_ADDRESS`
> I removed [[no_unique_address]] attribute because I ran into a trouble.
> in P2505R5, for the && and const&& overload of transform, the full wording of the paper is:
> ```
> template<class F> constexpr auto transform(F&& f) &&;
> template<class F> constexpr auto transform(F&& f) const &&;
> 
> Let U be remove_cv_t<invoke_result_t<F, decltype(std::move(value()))>>.
> 
> Constraints: is_move_constructible_v<E> is true.
> 
> Mandates: U is a valid value type for expected.
> If is_void_v<U> is false, the declaration U u(invoke(std::forward<F>(f), std::move(value())));
> is well-formed for some invented variable u.
> 
> Effects:
> 
>     (24.1)
>     If has_value() is false, returns expected<U, E>(unexpect, std::move(error())).
>     (24.2)
>     Otherwise, if is_void_v<U> is false, returns an expected<U, E> object whose has_val member is true and val member is `direct-non-list-initialized` with invoke(std::forward<F>(f), std::move(value())).
>     (24.3)
>     Otherwise, evaluates invoke(std::forward<F>(f), std::move(value())) and then returns expected<U, E>().
> 
> ```
> If we use named union, I have no idea to deal with 24.2:
> ```
>  Otherwise, if is_void_v<U> is false, returns an expected<U, E> object whose has_val member is true and val member is `direct-non-list-initialized` with invoke(std::forward<F>(f), std::move(value())).
> ```
> In https://reviews.llvm.org/D124516 I learned that [[no_unique_address]] cannot be used on anonymous unions, so I removed this attribute and changed named unions to anonymous unions.
> 
> If use anonymous union, I can implement the `direct-non-list initialized` sematic:
> ```
>   template <class _Func, class... _Args>
>   constexpr explicit expected(__expected::__expected_construct_in_place_from_invoke_tag, _Func&& __f, _Args&&... __args)
>       : __val_(_VSTD::invoke(_VSTD::forward<_Func>(__f), _VSTD::forward<_Args>(__args)...)), __has_val_(true) {}
> ```
> 
> The test case:
> ```
> constexpr void test_xval() {
>   struct NonCopyable {
>     constexpr NonCopyable(int i) : i(i) {}
>     NonCopyable(const NonCopyable&) = delete;
>     int i;
>   };
> 
>   auto xform = [](int i) { return NonCopyable(i); };
>   std::expected<int, int> e2(2);
>   std::expected<NonCopyable, int> n2 = e2.transform(xform);
>   assert(n2.value().i == 2);
> }
> 
> ```
> 
> 
You should be able to just `return expected<U, E>(in_place, std::invoke(...))`. I'm not sure why it's not written that way in the standard though.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140911