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

Tue Dec 13 09:28:20 PST 2022

ldionne accepted this revision.
ldionne added a comment.

@huixie90 Thank you so much for this great patch. It's not an easy one but I'm extremely happy with the state it has reached now, and I think we've addressed everyone's comments. I'm excited to ship this in LLVM 16!

I still left a few comments for minor nits, but feel free to ship this once they are addressed.

Thank you, and thanks to other folks who chimed in on the review!

================
Comment at: libcxx/include/__expected/expected.h:642-644
+    _LIBCPP_HIDE_FROM_ABI constexpr ~__union_t()
+      requires(!is_trivially_destructible_v<_Tp> || !is_trivially_destructible_v<_Err>)
+    {}
----------------

================
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))) {
----------------
jwakely wrote:
> 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).
Thanks for the ref and for schooling me :-).

================
Comment at: libcxx/test/libcxx/utilities/expected/expected.expected/value_or.mandates.verify.cpp:65
+    std::expected<NotConvertibleFromInt, int> f1{std::in_place};
+    std::move(f1).value_or(5);
+    //expected-error-re@*:* {{{{(static_assert|static assertion)}} failed {{.*}}argument has to be convertible to value_type}}
----------------
huixie90 wrote:
> ldionne wrote:
> > EricWF wrote:
> > > How do we know any of these diagnostics are triggered by the code above them? You need to add a check that at least some diagnostic is emitted from this line.
> > @huixie90  I would suggest moving this to the same line that you're expecting the error (i.e. current line 65) and dropping `*:*, which tells clang that the error can be on any line of any file (`file:line`). Like this:
> > 
> > ```
> > std::move(f1).value_or(5); // expected-error-re {{{{(static_assert|static assertion)}} failed {{.*}}argument has to be convertible to value_type}}
> > ```
> > 
> > That will make for a long line, but it does exactly what I think you want.
> For some reason, this does not work and the compiler thinks the error was emitted from expected.h instead of this particular line in the test
> 
> ```
> error: 'error' diagnostics expected but not seen: 
>   File /Users/huixie/repos/libc++/llvm-project/libcxx/test/libcxx/utilities/expected/expected.expected/value_or.mandates.verify.cpp Line 38: {{(static_assert|static assertion)}} failed {{.*}}value_type has to be copy constructible
> error: 'error' diagnostics seen but not expected: 
>   File /Users/huixie/repos/libc++/llvm-project/build/include/c++/v1/__expected/expected.h Line 595: static assertion failed due to requirement 'is_copy_constructible_v<NonCopyable>': value_type has to be copy constructible
> ```
I actually think that's expected, in retrospect. This is what happens for all of our `static_assert` tests. I think what you've got right now is fine, but if you wanted, you could also pin this down a bit more by:

1. Using `//expected-error-re at expected.h:*` to make sure that the `static_assert` doesn't come from e.g. `<vector>` (not sure how useful that is, but you could)
2.  Adding `//expected-note {{in instantiation}}` (you might have to figure out the right content inside `{{}}`) on the line of the `std::move(...).value_or(...)` call. That would ensure that Clang did indeed generate *some* diagnostic starting on that line. I think that would be way sufficient.

FWIW, I'm happy with the test as-is too.

================
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);
----------------
jwakely wrote:
> 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.
Thanks for confirming. @huixie90 Can you please add a link to the LWG issue in the comment above?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124516