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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/optional:602
+template <class _Tp>
+struct __is_optional : _VSTD::false_type {};
+template <class _Tp> struct __is_optional<_VSTD::optional<_Tp>> : _VSTD::true_type {};
----------------
We generally don't qualify types in namespace `std`. So just `false_type` would be consistent with what we do everywhere. This applies throughout the patch.


================
Comment at: libcxx/include/optional:1028
+        return _VSTD::invoke(_VSTD::forward<_Func>(__f), value());
+      return _VSTD::remove_cvref_t<__result_type> {};
+    }
----------------
Everywhere (for consistency).


================
Comment at: libcxx/include/optional:1045
+    constexpr auto and_then(_Func&& __f) && {
+      using __result_type = invoke_result_t<_Func, decltype(_VSTD::move(value()))>;
+      static_assert(__is_optional_v<__result_type>,
----------------
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.


================
Comment at: libcxx/include/optional:1068
+      using __result_type = remove_cvref_t<invoke_result_t<_Func, decltype(value())>>;
+      static_assert(!is_array_v<__result_type>, "Result of f(value()) should not be an Array");
+      static_assert(!is_same_v<__result_type, in_place_t>,
----------------
The paper says:

> Mandates: `U` is a non-array object type other than `in_place_t` or `nullopt_t`.

Here we're checking that it's not an array, it's not `in_place_t`, and it's not `nullopt_t`. But we are not checking that it's an object type. For instance, if `__result_type` ends up being a function type or `void`, we won't trigger any `static_assert`. I'm pretty sure it's going to fail in some other way, but I still think we should check for it explicitly.



================
Comment at: libcxx/include/optional:1074
+      if (*this)
+        return optional<__result_type> {_VSTD::invoke(_VSTD::forward<_Func>(__f), value())};
+      return optional<__result_type> {};
----------------
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/or_else.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Please add tests for SFINAE friendliness when the `Constraints:` are not met.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113408