[libcxx-commits] [PATCH] D124516: [libc++] Implement `std::expected` P0323R12

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a subscriber: curdeius.
ldionne added inline comments.


================
Comment at: libcxx/include/__expected/expected.h:75-89
+  static constexpr bool __can_convert =
+      is_constructible_v<_Tp, _Uf> &&                                     //
+      is_constructible_v<_Err, _Gf> &&                                    //
+      !is_constructible_v<_Tp, expected<_Up, _Gp>&> &&                    //
+      !is_constructible_v<_Tp, expected<_Up, _Gp>> &&                     //
+      !is_constructible_v<_Tp, const expected<_Up, _Gp>&> &&              //
+      !is_constructible_v<_Tp, const expected<_Up, _Gp>> &&               //
----------------
That will evaluate these `is_constructible<...>` expressions in a short-circuiting fashion.


================
Comment at: libcxx/include/__expected/expected.h:92
+  template <class _Gf>
+  static constexpr bool __can_assign_from_unexpected = //
+      is_constructible_v<_Err, _Gf> &&                 //
----------------
Same comment for laziness here. Should look like:

```
template <class _Gf>
static constexpr bool __can_assign_from_unexpected = _And<
  is_constructible<_Err, _Gf>,
  is_assignable<_Err&, _Gf>,
  _Lazy<_Or,
    is_nothrow_constructible<_Err, _Gf>,
    is_nothrow_move_constructible<_Tp>,
    is_nothrow_move_constructible<_Err>
  >
>::value;

I'm using `disjunction` (could also do `_Lazy<_Or, ...>`) because I don't want whatever's inside `_Or<...>` to be instantiated immediately.


================
Comment at: libcxx/include/__expected/expected.h:497
+    if (!__has_val_) {
+#  ifndef _LIBCPP_NO_EXCEPTIONS
+      throw bad_expected_access<_Err>(__unex_);
----------------
Let's define a `__throw_bad_expected_access` function like we do for many other exception types, and use it consistently. We can have the `#if #else` logic in a single place.


================
Comment at: libcxx/include/__expected/unexpected.h:59
+class unexpected {
+  static_assert(__unexpected::__valid_unexpected_error<_Err>);
+
----------------
Also, let's add similar messages to `std::expected`. As a user, I would really welcome this additional explanation.


================
Comment at: libcxx/include/__expected/unexpected.h:70
+  _LIBCPP_HIDE_FROM_ABI constexpr explicit unexpected(_Error&& __error) //
+      noexcept(is_nothrow_constructible_v<_Err, _Error>)
+      : __unex_(std::forward<_Error>(__error)) {}
----------------
We have discussed adding `noexcept` as an extension several times in the past, and I remember we've done it in a few places. This article explaining the Lakos principle is relevant: https://quuxplusone.github.io/blog/2018/04/25/the-lakos-rule/.

I think this here is fine. However, I would like to request that we add a new macro to clearly mark that this is an extension. We could call it `_LIBCPP_NOEXCEPT_EXT`. That will simply make it clear that it's an extension so we can grep for those.


================
Comment at: libcxx/include/__expected/unexpected.h:104
+
+  _LIBCPP_HIDE_FROM_ABI friend constexpr void swap(unexpected& __x, unexpected& __y) //
+      noexcept(noexcept(__x.swap(__y)))
----------------
ldionne wrote:
> Consider defining this one before `operator==` to make it closer to the member `swap`. That's also how it is in the spec, so bonus points for that.
I am not sure how much I like this `//` pattern because it's becoming very prevalent. I _really_ like the way you've formatted this though, but I wish clang-format could get that result without so much help. @philnik any opinions on whether we can make `clang-format` do what we want here? Or maybe @curdeius, I think you've worked on `clang-format` a bunch lately?


================
Comment at: libcxx/include/__expected/unexpected.h:104-109
+  _LIBCPP_HIDE_FROM_ABI friend constexpr void swap(unexpected& __x, unexpected& __y) //
+      noexcept(noexcept(__x.swap(__y)))
+    requires is_swappable_v<_Err>
+  {
+    __x.swap(__y);
+  }
----------------
Consider defining this one before `operator==` to make it closer to the member `swap`. That's also how it is in the spec, so bonus points for that.


================
Comment at: libcxx/include/__expected/unexpected.h:116
+template <class _Err>
+unexpected(_Err) -> unexpected<_Err>;
+
----------------
Is there a test for this deduction guide?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124516