[libc++] Implement `std::expected` P0323R12

[huixie90]

Summary

Implement std::expected https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p0323r12.html

Added tests

[ldionne]

libcxx/include/__expected/expected.h

55–57

IMO the simplicity of always having the same set of transitive includes outweighs the additional <cstdlib> include, especially since I think most users will end up including that header anyway. I would just include this header unconditionally.

620–621

Can we use [[no_unique_address]] here? Can you investigate how that attribute interacts with unions? Also, does it make sense to put it on the union itself?

870

Please make sure you check that this is explicit, i.e. an implicit conversion to bool should fail.

884

927–930

Let's match the order of members in the non-void case.

929

Same comment for no_unique_address.

libcxx/include/__expected/unexpected.h

71

Can you please add a test for this default template argument? See Jonathan's comment above explaining where this comes from.

120–121

Please ensure you have tests for this deduction guide.

libcxx/include/__type_traits/is_member_pointer.h

16–18

Likewise here, I wouldn't bother with the conditional include. We'll eventually be able to remove it anyway once all supported compilers implement the builtin.

libcxx/include/__type_traits/is_nothrow_constructible.h

15–18 ↗

(On Diff #477004)

Same

libcxx/include/__type_traits/is_void.h

17–20

Same

libcxx/include/expected

54

[jwakely]

libcxx/include/__expected/expected.h

55–57

Another option would be to use __builtin_abort() which requires no header.

[huixie90]

more CI

[huixie90]

CI

[huixie90]

libcxx/include/__expected/expected.h

620–621

Here is my finding.

For anonymous union (which is the case in the expected, I can't find a way to make [[no_unique_address]] to have any effect
https://godbolt.org/z/zebh4E94d

For a named union, if we apply [[no_unique_address]] on the union itself plus all the members of the union, we can save some space.
https://godbolt.org/z/rGoa7c47d

So I think we don't need to do anything here

[huixie90]

more CI

[huixie90]

address comments

[huixie90]

libcxx/include/__expected/expected.h

864

noexcept does not seem to do SFINAE here. I need to constrain it with requires
https://godbolt.org/z/E15jK4Ter

[huixie90]

CI

[huixie90]

next try

[huixie90]

next try CI

[ldionne]

I'm still going to request changes because of the no_unique_address thing, but this basically LGTM.

Thanks a lot for the great patch and the great collaboration on this review. I'm extremely happy with the patch and in particular with the fact that we carefully considered the testing coverage during our review of the class itself (e.g. order of operations in assignment, etc.).

libcxx/include/__expected/expected.h

55–57

That's interesting, I never thought of doing that. For this review, I think we should use std::abort to be consistent with what we do everywhere else. And then we should review all places in the library where we use std::abort (which is all the potentially-exception-throwing functions) and see whether we can/should use __builtin_abort instead.

620–621

I would suggest using a named union instead so that we can apply [[no_unique_address]] to the member.

For unnamed unions, it looks like the compiler is applying the attribute to the union type declaration (not the member declaration). Since [[no_unique_address]] applies only to member declarations, the attribute ends up being ignored when used on an unnamed union.

Also, once that's implemented, let's add a libcxx-specific test that this optimization kicks in as intended.

864

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 XFAILs and we'll remove them when we drop support for 15.

libcxx/test/libcxx/utilities/expected/expected.expected/noexcept.extension.compile.pass.cpp

19

libcxx/test/libcxx/utilities/expected/expected.expected/value_or.mandates.verify.cpp

50

To get rid of these spurious errors, I would look into this:

// ADDITIONAL_COMPILE_FLAGS: -Xclang -verify-ignore-unexpected=error

See for example libcxx/test/libcxx/atomics/atomics.syn/incompatible_with_stdatomic.verify.cpp.

66

@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.

libcxx/test/std/utilities/expected/expected.expected/assign/assign.copy.pass.cpp

284

libcxx/test/std/utilities/expected/expected.expected/assign/emplace.pass.cpp

52

Do we not need exceptions-related tests here to make sure we perform operations in the right order?

Please go through all the operations of both the void and the non-void specializations. This is a very high bar, but I think it's worth doing it.

libcxx/test/std/utilities/expected/expected.expected/ctor/ctor.u.pass.cpp

116–126

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?

libcxx/test/std/utilities/expected/expected.unexpected/class_mandates/const.compile.fail.cpp

1 ↗

(On Diff #478095)

Please make this into a verify.cpp. We're trying to move away from compile.fail.cpp tests because it's too easy to write a test that fails for an unrelated reason.

Applies to all.

[huixie90]

named union and no_unique_address

[huixie90]

tidy up

[huixie90]

tidy up

[jwakely]

libcxx/include/__expected/expected.h

864

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).

libcxx/test/std/utilities/expected/expected.expected/ctor/ctor.u.pass.cpp

116–126

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.

[huixie90]

CI

libcxx/test/libcxx/utilities/expected/expected.expected/value_or.mandates.verify.cpp

66

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

[huixie90]

address remaining comments

libcxx/test/std/utilities/expected/expected.expected/assign/emplace.pass.cpp

52

turns out that the emplace function requires is_nothrow_constructible_v so we don't need a runtime tests. (there are compile time tests above to test that if it can throw , there will be no emplace function)

[huixie90]

CI

[ldionne]

@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!

libcxx/include/__expected/expected.h

643–645

864

Thanks for the ref and for schooling me :-).

libcxx/test/libcxx/utilities/expected/expected.expected/value_or.mandates.verify.cpp

66

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@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.

libcxx/test/std/utilities/expected/expected.expected/ctor/ctor.u.pass.cpp

116–126

Thanks for confirming. @huixie90 Can you please add a link to the LWG issue in the comment above?

[huixie90]

rebase

[huixie90]

CI

[huixie90]

CI