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

Wed Jan 18 07:08:43 PST 2023

tcanens added inline comments.

================
Comment at: libcxx/include/__expected/expected.h:867-871
+   union {
+     __empty_t __empty_;
+     _Tp __val_;
+     _Err __unex_;
+  };
----------------
yronglin wrote:
> yronglin wrote:
> > yronglin wrote:
> > > philnik wrote:
> > > > yronglin wrote:
> > > > > philnik wrote:
> > > > > > yronglin wrote:
> > > > > > > philnik wrote:
> > > > > > > > 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.
> > > > > > > In https://reviews.llvm.org/D113408 , I learned that this implementation used to avoid SFINAE-unfriendly generic lambda in this way(don't try to instantiate its body during overload resolution.)
> > > > > > Ah, it's to avoid the extra move. Do you have a test case for that? If not, please add one. The comment about the SFINAE-unfriendly lambda means that we should make sure that the functions don't try to instantiate the lambda during overload resolution. Anyways, you can direct-non-list-initialize the member like it's done elsewhere, i.e. `std::construct_at(std::addressof(__union_.__val_), std::invoke(...));` in the constructor body.
> > > > > Thanks @philnik , I have a simple reproducer(https://godbolt.org/z/vYvYh7K17), `std::construct_at(std::addressof(__union_.__val_), std::invoke(...));`  caused compile failed in this case, but `expected(...): __val_(std::invoke(...)) {}` compile success.
> > > > Here is a working version: https://godbolt.org/z/aE5Y8o6e7
> > > > Ah, it's to avoid the extra move. Do you have a test case for that? If not, please add one. The comment about the SFINAE-unfriendly lambda means that we should make sure that the functions don't try to instantiate the lambda during overload resolution. Anyways, you can direct-non-list-initialize the member like it's done elsewhere, i.e. `std::construct_at(std::addressof(__union_.__val_), std::invoke(...));` in the constructor body.
> > > 
> > > Yes, thanks for your reminder I have added a test case to make sure that the functions don't try to instantiate the lambda during overload resolution.
> > > Here is a working version: https://godbolt.org/z/aE5Y8o6e7
> > 
> > Wow! what a great idea! thank you!
> > 
> > Here is a working version: https://godbolt.org/z/aE5Y8o6e7
> 
> Seems GCC trunk and clang trunk has different behavior https://godbolt.org/z/jGaGT8K8T , this code passed in clang but failed in gcc
Guaranteed elision into a potentially-overlapping subobject is unsettled (and it's not clear that it's implementable, given that the function is allowed to clobber the tail padding) - see https://github.com/itanium-cxx-abi/cxx-abi/issues/107.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140911