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

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

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.

Looks pretty good to me so far!



================
Comment at: libcxx/include/optional:605
+template<class _Tp>
+constexpr bool __is_optional_v = __is_optional<_Tp>::value;
+#endif
----------------
I suggest `__is_std_optional` for consistency with `__is_std_{span,array}`. I also think there's no need for the `__is_optional_v` variable template (again for consistency).

(Or, if you absolutely want the variable template for efficiency — which you shouldn't — then I'd say to get rid of the struct template and just specialize the variable template directly.)


================
Comment at: libcxx/include/optional:1034
+    constexpr auto and_then(_Func&& __f) const& {
+      using __result_type = invoke_result_t<_Func, decltype(value())>;
+      static_assert(__is_optional_v<__result_type>,
----------------
Style nit: The Standard calls this `U`, and I think we should call it `_Up`. `__lower_snake_case` looks too much like a local variable to me (and is also verboser than needed). (Alternatively, `_Rp`; but I think it makes sense to follow the Standard's `U`.)


================
Comment at: libcxx/include/optional:1095
+    constexpr auto transform(_Func&& __f) && {
+      using __result_type = remove_cvref_t<invoke_result_t<_Func, decltype(std::move(value()))>>;
+      static_assert(!is_array_v<__result_type>, "Result of f(std::move(value())) should not be an Array");
----------------
Here `decltype(std::move(value()))` is just `_Tp&&` (and ditto throughout, with cvref-qualification differences). IMO the shorter `_Tp&&` form should be used for clarity.
Also, I'm like 95% sure that `invoke_result_t<_Func, _Tp&&>` is just `invoke_result_t<_Func, _Tp>` (because it all boils down to `declval` stuff in the end, and `declval` can't distinguish xvalues from prvalues).


================
Comment at: libcxx/test/std/utilities/optional/optional.monadic/and_then.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
For each of the new methods, I'd like to see one simple test case each sanity-checking that
- `f` is not gratuitously copied (it might not even be copyable)
- `f`'s `operator()` is called with the correct constness and ref-qualification (e.g. `operator()(int) &` vs `operator()(int) const&&`)
- The argument to `f` is passed with the correct value category (e.g. `operator()(int&) const` vs `operator()(int&&) const`)
- The argument to `f` (that is, `value()`) is not gratuitously copied (it might not even be copyable)


================
Comment at: libcxx/test/std/utilities/optional/optional.monadic/and_then.pass.cpp:32
+
+  static_cast<const std::optional<int>&>(opt).and_then([](int) {
+    assert(false);
----------------
`static_cast<const std::optional<int>&>(opt)` could be `std::as_const(opt)`. However, that still doesn't help with the `const&&` cast, so maybe you just want to introduce a const version right at the top of the function?
```
std::optional<int> o;
const std::optional<int>& co = o;
auto never_called = [](int) { assert(false); return std::optional<long>(); };
o.and_then(never_called);
co.and_then(never_called);
std::move(o).and_then(never_called);
std::move(co).and_then(never_called);

o = 1;
assert(...
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113408