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

Implement LWG-3836 std::expected<bool, E1> conversion constructor expected(const expected<U, G>&) should take precedence over expected(U&&) with operator bool #3587

Merged
merged 6 commits into from Mar 28, 2023

Conversation

frederick-vs-ja
Copy link
Contributor

Fixes #3433.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner March 23, 2023 17:13
@github-actions github-actions bot added this to Initial Review in Code Reviews Mar 23, 2023
@StephanTLavavej StephanTLavavej added the LWG Library Working Group issue label Mar 24, 2023
@StephanTLavavej StephanTLavavej self-assigned this Mar 24, 2023
@@ -239,19 +239,23 @@ public:

template <class _Ty2>
using _AllowDirectConversion = bool_constant<conjunction_v<negation<is_same<_Remove_cvref_t<_Ty2>, optional>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Pre-existing, but why is this doing a bool_constant<conjunction_v rather than just conjunction; no change requested.

Copy link
Member

Choose a reason for hiding this comment

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

I think there's some attempt here to improve throughput by "smashing" a complicated type to either true_type or false_type but looking at this again, I am unsure whether we are actually achieving anything here. In cases below (e.g. _AllowUnwrappingAssignment) we are introducing a new struct instead of simply a using, and there appears to be no short-circuit benefit to doing so. I'll record a todo on my list, but I would prefer to avoid touching this existing code in <optional> until Casey returns as there is no urgent need.

@strega-nil-ms strega-nil-ms moved this from Initial Review to Final Review in Code Reviews Mar 24, 2023
Comment on lines +670 to +671
std::expected<bool, DerivedError> e1(false);
std::expected<bool, BaseError> e2(e1);
Copy link
Member

Choose a reason for hiding this comment

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

No change requested: This test is already using namespace std;. Not worth resetting testing as this test has already accumulated ~20 occurrences; I'll clean this up later, no reason to pollute this PR.

@StephanTLavavej StephanTLavavej removed their assignment Mar 24, 2023
@StephanTLavavej StephanTLavavej moved this from Final Review to Ready To Merge in Code Reviews Mar 24, 2023
@StephanTLavavej StephanTLavavej self-assigned this Mar 28, 2023
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit df2af9e into microsoft:main Mar 28, 2023
35 checks passed
Code Reviews automation moved this from Ready To Merge to Done Mar 28, 2023
@StephanTLavavej
Copy link
Member

Thanks for implementing one of the most verbosely-titled LWG issues ever! 😹 ✅ 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LWG Library Working Group issue
Projects
No open projects
3 participants