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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik 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>> &&               //
----------------
ldionne wrote:
> That will evaluate these `is_constructible<...>` expressions in a short-circuiting fashion.
What exactly is the benefit of short-circuiting here? Is it required to satisfy the spec? If not, are you sure it's worth instantiating types and short-circuiting instead of using variables? I think instantiating types takes quite a bit longer, but I might be wrong on that.


================
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)) {}
----------------
ldionne wrote:
> 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.
I think we actually have `noexcept` extensions in quite a few places. What exactly is the benefit of adding a new macro for this? I don't think we ever want to disable it, and I can't really imagine a scenario where you would be interested in `noexcept` extensions specifically. From what I can tell, adding a macro just makes the code harder to read.


================
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:
> 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?
What exactly are you asking for?
```lang=c++
  _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);
  }
```
seems just fine to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124516