Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

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

Authored by huixie90 on Apr 27 2022, 3:33 AM.

Details

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ldionne added inline comments.Nov 23 2022, 10:35 AM
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 added inline comments.Nov 23 2022, 11:17 AM
libcxx/include/__expected/expected.h
55–57

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

huixie90 updated this revision to Diff 477836.Nov 24 2022, 12:56 PM
huixie90 marked 19 inline comments as done.

more CI

huixie90 added inline comments.Nov 24 2022, 2:57 PM
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 updated this revision to Diff 477981.Nov 25 2022, 8:48 AM
huixie90 marked 11 inline comments as done.

address comments

huixie90 added inline comments.Nov 25 2022, 9:41 AM
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 updated this revision to Diff 478095.Nov 27 2022, 6:37 AM

next try CI

ldionne requested changes to this revision.Nov 29 2022, 10:23 AM
ldionne added a subscriber: jfb.

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.

This revision now requires changes to proceed.Nov 29 2022, 10:23 AM
huixie90 updated this revision to Diff 478682.Nov 29 2022, 12:35 PM

named union and no_unique_address

jwakely added inline comments.Nov 30 2022, 1:23 AM
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 updated this revision to Diff 479077.Nov 30 2022, 2:46 PM
huixie90 marked 9 inline comments as done.

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 updated this revision to Diff 479847.Dec 3 2022, 9:44 AM
huixie90 marked 5 inline comments as done.

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)

ldionne accepted this revision.Dec 13 2022, 9:28 AM

@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 updated this revision to Diff 482792.Dec 14 2022, 3:33 AM
huixie90 marked 11 inline comments as done.

rebase

This revision was not accepted when it landed; it landed in state Needs Review.Dec 14 2022, 7:45 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.