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

[Jonathan Wakely via Phabricator via libcxx-commits]

jwakely added inline comments.


================
Comment at: libcxx/include/__expected/expected.h:863
+
+  _LIBCPP_HIDE_FROM_ABI friend constexpr void swap(expected& __x, expected& __y) //
+      noexcept(noexcept(__x.swap(__y))) {
----------------
ldionne wrote:
> huixie90 wrote:
> > ldionne wrote:
> > > Please make sure you have tests that check that this is "equivalent to" the member swap, i.e. that the same constraints apply. This should be SFINAE friendly if the member swap can't be used, basically.
> > `noexcept`  does not seem to do SFINAE here. I need to constrain it with `requires`
> > https://godbolt.org/z/E15jK4Ter
> I stand corrected. I was *certain* `noexcept` was an immediate context that you could SFINAE on. TIL.
> 
> I think what you have now with `requires requires` is right. It's OK if it fails with clang-15, let's just add the appropriate `XFAIL`s and we'll remove them when we drop support for 15.
> I stand corrected. I was *certain* `noexcept` was an immediate context that you could SFINAE on. TIL.

No, [temp.deduct.general] p7 is very explicit about that. `noexcept` doesn't participate in overload resolution, and doesn't even need to be instantiated unless checked ([temp.inst] p14).


================
Comment at: libcxx/test/std/utilities/expected/expected.expected/ctor/ctor.u.pass.cpp:115-125
+  // this is a confusing example, but the behaviour
+  // is exactly what is specified in the spec
+  {
+    struct BaseError {};
+    struct DerivedError : BaseError {};
+
+    std::expected<bool, BaseError> e1(false);
----------------
ldionne wrote:
> huixie90 wrote:
> > @jwakely Hi Jon, thank you very much for commenting on this patch and they are really helpful. While you are here, what do you think about this test case? If I switch `bool` to `int`, 
> > ```
> > std::expected<int, BaseError> e1(5);
> > std::expected<int, DerivedError> e2(e1);
> > ```
> > 
> > the other ctor overload
> > 
> > ```
> > expected(const expected<U, G>&)
> > ```
> > will be selected and the `int` value `5` will be copied. 
> > 
> > But for `bool`, as `T` (`bool`) is constructible from the `expected`, the other overload will be disabled and this overload will be selected
> > ```
> > expected(U&&);
> > ```
> > so instead of copying the `bool` (`false`), it uses `operator bool`, which is `true` this case.
> > 
> > Do you think this behaviour is confusing and not very consistent
> This seems surprising to me. It seems like this might not have been the design intent. @jfb perhaps you can comment on this?
> 
> Is this worth a LWG issue?
The rules are supposed to allow initialization of a result type that can be constructed from `expected`, but because `expected<T,U>` is always contextually convertible to `bool`, that kicks in here. This reminds me of https://wg21.link/p0608 where we also needed to exclude `bool` from being greedy. It shouldn't be a problem for anything other than `bool`.

Please do file a library issue, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124516