[libcxx-commits] [PATCH] D140911: [libc++] Implement P2505R5(Monadic operations for std::expected).
Nikolas Klauser via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Mar 27 02:39:49 PDT 2023
philnik added inline comments.
================
Comment at: libcxx/include/__expected/expected.h:867-871
+ union {
+ __empty_t __empty_;
+ _Tp __val_;
+ _Err __unex_;
+ };
----------------
yronglin wrote:
> philnik wrote:
> > yronglin wrote:
> > > tcanens wrote:
> > > > 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.
> > > Many Thanks for your reminder @tcanens ! Learned from this discussion, C++ standard still requires do direct initialization, but that impossible in current ABI, so, gcc says this code ( https://godbolt.org/z/K6edrdTG5) is ill-formed, but if we remove [[no_unique_address]], that works again. For the usability and robustness of `std::expected`, I think should we remove [[no_unique_address]] attribute until CWG2403 resolved this issue? Of course this will loses the benefit of [[no_unique_address]], and I don't have a strong opinion either way. I believe you have more experience than me. what do you all think?
> > We can't just apply `[[no_unique_address]]` later, since it would be an ABI break. We could do something like
> > ```lang=c++
> > union __union_t requires (is_trivially_move_constructible_v<_Tp> && is_trivially_move_constructible_v<_Err>) {
> > _LIBCPP_NO_UNIQUE_ADDRESS _Tp __val_;
> > _LIBCPP_NO_UNIQUE_ADDRESS _Err __unex_;
> > ...
> > };
> >
> > union __union_t {
> > _Tp __val_;
> > _Err __unex_;
> > ...
> > };
> >
> > _LIBCPP_NO_UNIQUE_ADDRESS __union_t __union_;
> > ```
> > That shouldn't be observable. @ldionne @huixie90 WDYT?
> @philnik @ldionne @huixie90 WDYT?
>
> ```
> struct __empty_t {};
>
> template <class _ValueType, class _ErrorType>
> union __union_t {
> constexpr __union_t() : __empty_() {}
>
> template <class _Func, class... _Args>
> _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(
> std::__expected_construct_in_place_from_invoke_tag, _Func&& __f, _Args&&... __args)
> : __val_(std::invoke(std::forward<_Func>(__f), std::forward<_Args>(__args)...)) {}
>
> template <class _Func, class... _Args>
> _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(
> std::__expected_construct_unexpected_from_invoke_tag, _Func&& __f, _Args&&... __args)
> : __unex_(std::invoke(std::forward<_Func>(__f), std::forward<_Args>(__args)...)) {}
>
> _LIBCPP_HIDE_FROM_ABI constexpr ~__union_t()
> requires(is_trivially_destructible_v<_ValueType> && is_trivially_destructible_v<_ErrorType>)
> = default;
>
> // the expected's destructor handles this
> _LIBCPP_HIDE_FROM_ABI constexpr ~__union_t()
> requires(!is_trivially_destructible_v<_ValueType> || !is_trivially_destructible_v<_ErrorType>)
> {}
>
> __empty_t __empty_;
> _ValueType __val_;
> _ErrorType __unex_;
> };
>
> // use named union because [[no_unique_address]] cannot be applied to an unnamed union,
> // also 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.
> template <class _ValueType, class _ErrorType>
> requires(is_trivially_move_constructible_v<_ValueType> && is_trivially_move_constructible_v<_ErrorType>)
> union __union_t<_ValueType, _ErrorType> {
> _LIBCPP_HIDE_FROM_ABI constexpr __union_t() : __empty_() {}
>
> template <class _Func, class... _Args>
> _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(
> std::__expected_construct_in_place_from_invoke_tag, _Func&& __f, _Args&&... __args)
> : __val_(std::invoke(std::forward<_Func>(__f), std::forward<_Args>(__args)...)) {}
>
> template <class _Func, class... _Args>
> _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(
> std::__expected_construct_unexpected_from_invoke_tag, _Func&& __f, _Args&&... __args)
> : __unex_(std::invoke(std::forward<_Func>(__f), std::forward<_Args>(__args)...)) {}
>
> _LIBCPP_HIDE_FROM_ABI constexpr ~__union_t()
> requires(is_trivially_destructible_v<_ValueType> && is_trivially_destructible_v<_ErrorType>)
> = default;
>
> // the expected's destructor handles this
> _LIBCPP_HIDE_FROM_ABI constexpr ~__union_t()
> requires(!is_trivially_destructible_v<_ValueType> || !is_trivially_destructible_v<_ErrorType>)
> {}
>
> _LIBCPP_NO_UNIQUE_ADDRESS __empty_t __empty_;
> _LIBCPP_NO_UNIQUE_ADDRESS _ValueType __val_;
> _LIBCPP_NO_UNIQUE_ADDRESS _ErrorType __unex_;
> };
>
> _LIBCPP_NO_UNIQUE_ADDRESS __union_t<_Tp, _Err> __union_;
> bool __has_val_;
> ```
Looks good from my side, but @ldionne and @huixie90 should also take a look.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140911/new/
https://reviews.llvm.org/D140911
More information about the libcxx-commits
mailing list