[libcxx-commits] [PATCH] D154116: [libc++] Implement LWG3938 (Cannot use std::expected monadic ops with move-only error_type)

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/docs/Status/Cxx2cIssues.csv:17
 "`3935 <https://wg21.link/LWG3935>`__","``template<class X> constexpr complex& operator=(const complex<X>&)`` has no specification","Varna June 2023","|Complete|","3.4",""
-"`3938 <https://wg21.link/LWG3938>`__","Cannot use ``std::expected`` monadic ops with move-only ``error_type``","Varna June 2023","","",""
+"`3938 <https://wg21.link/LWG3938>`__","Cannot use ``std::expected`` monadic ops with move-only ``error_type``","Varna June 2023","|Complete|","17.0",""
 "`3940 <https://wg21.link/LWG3940>`__","``std::expected<void, E>::value()`` also needs ``E`` to be copy constructible","Varna June 2023","","",""
----------------
I think this should be `18.0` now.


================
Comment at: libcxx/test/std/utilities/expected/expected.expected/monadic/and_then.pass.cpp:173
+// [LWG 3983] https://cplusplus.github.io/LWG/issue3938, check std::expected monadic ops well-formed with move-only error_type.
+static_assert(has_and_then<std::expected<int, CopyConstructibleErrorType>&, std::expected<int, CopyConstructibleErrorType>(int&)>);
+// There are no effects for `const &` overload, because the constraints requires is_constructible_v<E, decltype(error())> is true.
----------------
Here and for the other monadic operations, I think we should add runtime tests that use a move-only error type. Otherwise, if we had something like

```
if (error-is-move-only) {
  std::abort();
}
```

in the implementation of e.g. `and_then`, the current tests would pass but out implementation would obviously be incorrect. Generally speaking, `static_assert` tests are really nice in their negative form, to make sure that something is SFINAE friendly. However, just checking that things compile for a positive test is usually insufficient.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154116