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

Tim Song via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Sun Nov 14 14:17:31 PST 2021


tcanens added inline comments.


================
Comment at: libcxx/include/optional:1092
+  constexpr auto transform(_Func&& __f) & {
+    using _Up = remove_cvref_t<invoke_result_t<_Func, value_type&>>;
+    static_assert(!is_array_v<_Up>, "Result of f(value()) should not be an Array");
----------------
This should be `remove_cv_t`. Ditto for other instances of `transform`.


================
Comment at: libcxx/include/optional:1153
+    static_assert(is_same_v<remove_cvref_t<invoke_result_t<_Func>>, optional>,
+                  "Result of f() should be an optional");
+    if (*this)
----------------
Not just an `optional` - it needs to be this particular specialization of `optional`.


================
Comment at: libcxx/include/optional:1074
+      if (*this)
+        return optional<__result_type> {_VSTD::invoke(_VSTD::forward<_Func>(__f), value())};
+      return optional<__result_type> {};
----------------
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.
> 




================
Comment at: libcxx/test/std/utilities/optional/optional.monadic/and_then.pass.cpp:8
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
----------------
For the ones returning `auto`, I'd suggest a test using a SFINAE-unfriendly generic lambda to verify that they don't try to instantiate its body during overload resolution. See http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p0798r8.html#sfinae-friendliness



================
Comment at: libcxx/test/std/utilities/optional/optional.monadic/transform.pass.cpp:129
+  std::move(nc).transform(NoCopy{});
+
+  return true;
----------------
This seems to be missing a test where the transform function actually returns something immovable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113408



More information about the libcxx-commits mailing list