[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 a comment.
This revision now requires changes to proceed.

Almost LGTM, thanks for working on this!



================
Comment at: libcxx/include/optional:135-145
+    // [optional.monadic], monadic operations
+    template<class F> constexpr auto and_then(F&& f) &;         // since c++23
+    template<class F> constexpr auto and_then(F&& f) &&;        // since c++23
+    template<class F> constexpr auto and_then(F&& f) const&;    // since c++23
+    template<class F> constexpr auto and_then(F&& f) const&&;   // since c++23
+    template<class F> constexpr auto transform(F&& f) &;        // since c++23
+    template<class F> constexpr auto transform(F&& f) &&;       // since c++23
----------------



================
Comment at: libcxx/include/optional:616
 
+#if _LIBCPP_STD_VER > 20
+template<class _Tp>
----------------
I think it is fine to unconditionally define this template. We'll only be using it in C++20, but I don't think it's worth the complexity to add the `#if`.


================
Comment at: libcxx/include/optional:1060
+    using _Up = invoke_result_t<_Func, const value_type&>;
+    static_assert(__is_std_optional<_Up>::value,
+                  "Result of f(value()) must be a specialization of std::optional<T>");
----------------
The spec says:

> Let `U` be `invoke_result_t<F, decltype(std::move(value()))>`.
> Mandates: `remove_cvref_t<U>` is a specialization of `optional`.

I think we should be checking this instead:

```
static_assert(__is_std_optional<remove_cvref_t<_Up>>::value, ...);
```

This comment probably applies elsewhere.


================
Comment at: libcxx/test/std/utilities/optional/optional.monadic/or_else.pass.cpp:25-32
+template <class O, class F>
+requires requires(O&& o, F&& f) { {std::forward<O>(o).or_else(std::forward<F>(f))}; }
+constexpr bool has_or_else(O&&, F&&) { return true; }
+
+template <class O, class F>
+constexpr bool has_or_else(O&&, F&&) {
+  return false;
----------------
I believe this would work and be equivalent:

```
template <class Opt, class F>
concept has_or_else = requires(Opt&& opt, F&& f) {
  { std::forward<Opt>(opt).or_else(std::forward<F>(f)) };
};
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113408