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

[Yurong via Phabricator via libcxx-commits]

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

Thanks for your reply!



================
Comment at: libcxx/include/__expected/expected.h:75
   (void)__arg;
-  std::abort();
+  _VSTD::abort();
 #  endif
----------------
philnik wrote:
> 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.
Thanks, I will undo this change and keep the same style as the original code(use `std::`)


================
Comment at: libcxx/include/__expected/expected.h:867-871
+   union {
+     __empty_t __empty_;
+     _Tp __val_;
+     _Err __unex_;
+  };
----------------
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140911