[libcxx-commits] [PATCH] D113408: [libc++] Implement P0798R8 (Monadic operations for std::optional)

[Tim Song via Phabricator via libcxx-commits]

tcanens added inline comments.


================
Comment at: libcxx/include/optional:1074
+      if (*this)
+        return optional<__result_type> {_VSTD::invoke(_VSTD::forward<_Func>(__f), value())};
+      return optional<__result_type> {};
----------------
tcanens wrote:
> ldionne wrote:
> > This isn't what the paper is expressing. The full wording of the paper is:
> > 
> > > Let `U` be `remove_cv_t<invoke_result_t<F, decltype(value())>>`.
> > >
> > > Mandates: `U` is a non-array object type other than `in_place_t` or `nullopt_t`. The declaration `U u(invoke(std::forward<F>(f), value()));` is well-formed for some invented variable `u`. [Note: `is_move_constructible_v<U>` can be `false`. -- end note]
> > >
> > > Returns: If `*this` does not contain a value, `optional<U>();` otherwise, an `optional<U>` object whose contained value is initialized as if direct-non-list-initializing an object of type `U` with `invoke(std::forward<F>(f), value())`.
> > 
> > What they mean in the `Returns:` section is that the object inside the optional should be direct-non-list-initialized as-if one had used `U u(invoke(std::forward<F>(f), value()));` exactly. Combined with the note that `U` doesn't have to be move-constructible, I am fairly convinced they are telling us to allow move-elision, like what I did in D107932. At the end of the day, I suspect this will have to look something like
> > 
> > ```
> > optional<__result_type>(__construct_from_invoke_tag{}, _VSTD::forward<_Func>(__f), value());
> > ```
> > 
> > where there's a special `optional` constructor that looks like
> > 
> > ```
> > template<class _Fp, class ..._Args>
> > constexpr explicit optional(__construct_from_invoke_tag, _Fp&& __f, _Args&& ...__args) 
> >   : __value_(_VSTD::invoke(_VSTD::forward<_Fp>(__f), _VSTD::forward<_Args>(__args)...)))
> > { }
> > ```
> > 
> > This is of course going to be more complicated due to inheritance and stuff, but you get the idea. Here, at least, we allow move elision to occur.
> > 
> > 
> > Also, please add tests for all the methods that have this `Note:` where `__result_type` is not move-constructible. Those should fail with your current implementation and they should start working once you implement something like what I just laid out.
> > 
> > This isn't what the paper is expressing. The full wording of the paper is:
> > 
> > > Let `U` be `remove_cv_t<invoke_result_t<F, decltype(value())>>`.
> > >
> > > Mandates: `U` is a non-array object type other than `in_place_t` or `nullopt_t`. The declaration `U u(invoke(std::forward<F>(f), value()));` is well-formed for some invented variable `u`. [Note: `is_move_constructible_v<U>` can be `false`. -- end note]
> > >
> > > Returns: If `*this` does not contain a value, `optional<U>();` otherwise, an `optional<U>` object whose contained value is initialized as if direct-non-list-initializing an object of type `U` with `invoke(std::forward<F>(f), value())`.
> > 
> > What they mean in the `Returns:` section is that the object inside the optional should be direct-non-list-initialized as-if one had used `U u(invoke(std::forward<F>(f), value()));` exactly. Combined with the note that `U` doesn't have to be move-constructible, I am fairly convinced they are telling us to allow move-elision, like what I did in D107932. At the end of the day, I suspect this will have to look something like
> > 
> > ```
> > optional<__result_type>(__construct_from_invoke_tag{}, _VSTD::forward<_Func>(__f), value());
> > ```
> > 
> > where there's a special `optional` constructor that looks like
> > 
> > ```
> > template<class _Fp, class ..._Args>
> > constexpr explicit optional(__construct_from_invoke_tag, _Fp&& __f, _Args&& ...__args) 
> >   : __value_(_VSTD::invoke(_VSTD::forward<_Fp>(__f), _VSTD::forward<_Args>(__args)...)))
> > { }
> > ```
> > 
> > This is of course going to be more complicated due to inheritance and stuff, but you get the idea. Here, at least, we allow move elision to occur.
> > 
> > 
> > Also, please add tests for all the methods that have this `Note:` where `__result_type` is not move-constructible. Those should fail with your current implementation and they should start working once you implement something like what I just laid out.
> > 
> 
> 
I thought I dropped this one... anyway, I wrote this wording and yes, that's the intent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113408