Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<expected> Implement P0323R12 #2643

Merged
merged 29 commits into from May 24, 2022
Merged

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Apr 8, 2022

This implements <expected> that has been added by P0323R12.

I have also incorporated the changes from WG21-P2549.

I might have been a bit too strict with constraints.

Fixes #2529

@miscco miscco requested a review from a team as a code owner April 8, 2022 21:02
stl/inc/expected Outdated Show resolved Hide resolved
stl/inc/expected Outdated Show resolved Hide resolved
stl/inc/expected Outdated Show resolved Hide resolved
tests/std/tests/P0323R12_expected/test.cpp Show resolved Hide resolved
stl/inc/expected Outdated Show resolved Hide resolved
@miscco

This comment was marked as resolved.

@AlexGuteniev
Copy link
Contributor

Have you considered empty object optimization for Empty unexpected class?

@miscco
Copy link
Contributor Author

miscco commented Apr 10, 2022

Have you considered empty object optimization for Empty unexpected class?

You mean in the case expected<void, empty>?

@AlexGuteniev
Copy link
Contributor

Have you considered empty object optimization for Empty unexpected class?

You mean in the case expected<void, empty>?

Yes, and also expected<int, empty>.

@miscco
Copy link
Contributor Author

miscco commented Apr 10, 2022

Have you considered empty object optimization for Empty unexpected class?

You mean in the case expected<void, empty>?

Yes, and also expected<int, empty>.

But the second one would not benefit from any empty base class optimization as we still have to allocate the value of T.

Also this essentially doubles the implementation size because we still have to do a lot of things. E.g. just because a type is empty does not mean its construction is trivial. So we would need to still keep a lot of the implementation.

I think if the maintainers really want to one could do it but the gains seem questionable.

@StephanTLavavej StephanTLavavej added the cxx23 C++23 feature label Apr 11, 2022
@StephanTLavavej StephanTLavavej added this to Initial Review in Code Reviews via automation Apr 11, 2022
@frederick-vs-ja

This comment was marked as resolved.

@miscco

This comment was marked as resolved.

@StephanTLavavej StephanTLavavej self-assigned this Apr 13, 2022
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Video code review pending. 😸

Note to self: I still need to review <expected> starting at class expected, and the new test.cpp.

stl/inc/yvals_core.h Show resolved Hide resolved
tests/std/tests/P0323R12_expected/env.lst Show resolved Hide resolved
tests/std/test.lst Show resolved Hide resolved
stl/inc/expected Show resolved Hide resolved
stl/inc/expected Outdated Show resolved Hide resolved
stl/inc/expected Show resolved Hide resolved
stl/inc/expected Outdated Show resolved Hide resolved
stl/inc/expected Outdated Show resolved Hide resolved
stl/inc/expected Outdated Show resolved Hide resolved
stl/inc/expected Outdated Show resolved Hide resolved
Code Reviews automation moved this from Initial Review to Work In Progress Apr 15, 2022
@StephanTLavavej StephanTLavavej removed their assignment May 20, 2022
@StephanTLavavej StephanTLavavej moved this from Initial Review to Final Review in Code Reviews May 20, 2022
@CaseyCarter CaseyCarter self-assigned this May 20, 2022
Thanks to cplusplus/draft#5381 ,
this shouldn't be marked as strengthened.
@StephanTLavavej
Copy link
Member

I've pushed one more change thanks to @frederick-vs-ja's discovery in #2733 that cplusplus/draft#5381 has patched expected::error() to always be noexcept in the Standard (for both the primary template and void specialization).

@StephanTLavavej
Copy link
Member

@AlexGuteniev Thanks for raising the Empty Base Class Optimization question (in #2643 (comment) etc). I agree with @miscco that the implementation complexity would be high and the benefits would be minimal. Specifically, only the following cases would benefit:

  • expected<EmptyType, EmptyError>
  • expected<void, EmptyError>

In these cases, we could reduce the representation from 2 bytes (empty union followed by bool) to 1 byte (just the bool). Although it halves the space consumption, these scenarios seem quite unlikely. expected is useful when the error type contains a "reason". If it's an empty error type, it's just signaling its presence, so expected<Anything, EmptyError> provides little value over optional<Anything> (either you have the result or you don't), and expected<void, EmptyError> is basically a bool (did you succeed or fail - expected is slightly less ambiguous, but an enum class Result : bool { Failure, Success }; would avoid ambiguity.

Unlike empty allocators, empty comparators, empty function objects, etc. I think that the EBCO isn't worth the implementation complexity, throughput cost, and risk here.

@StephanTLavavej StephanTLavavej self-assigned this May 21, 2022
@StephanTLavavej
Copy link
Member

I'm speculatively mirroring this to the MSVC-internal repo - further changes can be pushed, but please notify me.

"Standard Library Header Units: <expected> ICEs with Assertion failed: IsInClassDefn()"
@StephanTLavavej
Copy link
Member

I had to push an additional change to work around VSO-1543660 "Standard Library Header Units: <expected> ICEs with Assertion failed: IsInClassDefn()" by disabling the use of <expected> in that test. Additionally, I have renamed expected local variables to avoid shadowing when the test coverage is restored (interestingly, merely import <expected>; followed by such shadowing variables was causing the ICE).

Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed product code only. I'm now going to review the test code, after which I'll prepare changes for my comments if nobody has commented otherwise. (I'm trying to avoid digging through 175 comments to see if any of these have already been rejected, but I'll probably dig through anyway before I push.)

stl/debugger/STL.natvis Show resolved Hide resolved
stl/inc/expected Outdated Show resolved Hide resolved
stl/inc/expected Outdated Show resolved Hide resolved
stl/inc/expected Show resolved Hide resolved
stl/inc/expected Show resolved Hide resolved
stl/inc/expected Outdated Show resolved Hide resolved
stl/inc/expected Outdated Show resolved Hide resolved
stl/inc/expected Show resolved Hide resolved
stl/inc/expected Outdated Show resolved Hide resolved
stl/inc/expected Outdated Show resolved Hide resolved
Code Reviews automation moved this from Final Review to Work In Progress May 23, 2022
@CaseyCarter CaseyCarter removed their assignment May 23, 2022
@CaseyCarter CaseyCarter moved this from Work In Progress to Ready To Merge in Code Reviews May 23, 2022
@@ -2038,6 +2040,29 @@ namespace test_expected {
}
} // namespace test_expected

void test_reinit_regression() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have preferred to have this as part of the assignment tests but meh...

@StephanTLavavej
Copy link
Member

I have pushed a (hopefully final) commit to guard/optimize destruction as noticed by @miscco, and to enable internal coverage for the header units test now that @cdacamar has fixed the bug 🎉 🐞.

@StephanTLavavej StephanTLavavej merged commit b7ba3ad into microsoft:main May 24, 2022
Code Reviews automation moved this from Ready To Merge to Done May 24, 2022
@StephanTLavavej
Copy link
Member

Thanks for implementing this major C++23 feature - it's expected to be a popular one! 😹 😻 🎉

@miscco miscco deleted the p0323-expected branch May 24, 2022 11:43
fsb4000 pushed a commit to fsb4000/STL that referenced this pull request Aug 13, 2022
Co-authored-by: Stephan T. Lavavej <stl@microsoft.com>
Co-authored-by: Casey Carter <Casey@Carter.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx23 C++23 feature
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

P0323R12 <expected>