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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision.
Quuxplusone added a comment.
This revision is now accepted and ready to land.

I suspect it may be important to add some tests involving `optional<optional<int>>` just to make sure we strip (or don't strip) the right number of levels of `optional`. (This is one of the pernicious cases for CTAD, where the expression `std::optional(e)` changes meaning depending on the type of `e`.)  However, please don't block this PR over that; just consider whether you agree, and then make a followup PR with some additional tests if you think it's important enough to test. (I'm also willing to believe it's not so important, if that's what you think.)



================
Comment at: libcxx/include/optional:256-257
+  constexpr __optional_destruct_base(__optional_construct_from_invoke_tag, _Fp&& __f, _Args&&... __args)
+      : __val_(_VSTD::invoke(_VSTD::forward<_Fp>(__f), _VSTD::forward<_Args>(__args)...)), __engaged_(true) {
+  }
+#endif
----------------
Nit: match `{}` brace style from lines 244 and 250. (Ditto line 299.)


================
Comment at: libcxx/include/optional:1059
+    static_assert(__is_std_optional<remove_cvref_t<_Up>>::value,
+                  "Result of f(value()) must be a specialization of std::optional<T>");
+    if (*this)
----------------
I wonder if this should say `a specialization of std::optional`, without the `T`, for pedantic correctness. This can totally be done post-commit, though, if we want to, rather than holding up this PR waiting for a round-trip to see if @ldionne agrees or whatever. :)


================
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)
----------------
tcanens wrote:
> Not just an `optional` - it needs to be this particular specialization of `optional`.
Please make this `Result of f() should be the same type as this optional` — note the addition of "type." It might not be "the same (object)" in the same sense as `operator=`'s return type must be "the same as this optional." Adding `type` makes that unambiguous, and it's cheap to do.
Ditto line 1161.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113408