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

Avoid constraint recursion for expected<any, ...> and expected<void, any> #4013

Merged
merged 5 commits into from
Sep 7, 2023

Conversation

achabense
Copy link
Contributor

Fixes #4011.

@achabense achabense requested a review from a team as a code owner September 6, 2023 12:22
@achabense
Copy link
Contributor Author

achabense commented Sep 6, 2023

additional test
#include<any>
#include<expected>
#include<functional>

using namespace std;

void test_A() {
    using T = expected<any, int>;
    T t1;
    T t2{ move(t1) };
    T t3{ t2 };

    using U = expected<void, any>;
    U u1;
    U u2{ move(u1) };
    U u3{ u2 };
}

using X = expected<any, int>;
X meow() {
    return {};
}

void test_B() {
    function<X()> f{ meow };
}

int main() {
    test_A();
    test_B();
}

@CaseyCarter CaseyCarter added the bug Something isn't working label Sep 6, 2023
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.

Some comments that I'll test-and-push changes for. Thanks for jumping on this bug so fast!

stl/inc/expected Outdated Show resolved Hide resolved
stl/inc/expected Outdated Show resolved Hide resolved
tests/std/tests/P0323R12_expected/test.cpp Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej self-assigned this Sep 6, 2023
stl/inc/expected Show resolved Hide resolved
stl/inc/expected Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

Thanks! @CaseyCarter I pushed a trivial stylistic change after you approved.

@StephanTLavavej StephanTLavavej removed their assignment Sep 6, 2023
@StephanTLavavej StephanTLavavej self-assigned this Sep 7, 2023
@StephanTLavavej
Copy link
Member

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

&& !is_constructible_v<unexpected<_Err>, expected<_Uty, _UErr>&> //
&& !is_constructible_v<unexpected<_Err>, expected<_Uty, _UErr>> //
&& !is_constructible_v<unexpected<_Err>, const expected<_Uty, _UErr>&> //
&& !is_constructible_v<unexpected<_Err>, const expected<_Uty, _UErr>>;
Copy link
Contributor Author

@achabense achabense Sep 7, 2023

Choose a reason for hiding this comment

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

Sorry for my untimely reply; if it's late to make new pushes and these changes look ok, I will carry these changes to another pr.

  1. I think it's beneficial not to relocate _Allow_unwrapping here and in expected<void, ...>, as it is used closely after definition so its function can be more obvious.
  2. These can also be private:
    struct _NODISCARD _GuardTy {

    static constexpr void _Reinit_expected(_First& _New_val, _Second& _Old_val, _Args&&... _Vals) noexcept(

@achabense
Copy link
Contributor Author

achabense commented Sep 7, 2023

I find another weird case... t contains U instead of int after construction; I think this is a bit counterintuitive...
(gcc and clang also think t should contain U)

#include<any>
#include<expected>
using namespace std;

int main() {
    using T = expected<any, int>;
    using U = expected<int, int>;

    T t{ U{} };
    (void) any_cast<U>(t.value()); // not thrown, t.any contains U instead of int
}

@frederick-vs-ja
Copy link
Contributor

I find another weird case... t contains U instead of int after construction; I think this is a bit counterintuitive... (gcc and clang also think t should contain U)

#include<any>
#include<expected>
using namespace std;

int main() {
    using T = expected<any, int>;
    using U = expected<int, int>;

    T t{ U{} };
    (void) any_cast<U>(t.value()); // not thrown, t.any contains U instead of int
}

This is probably mandatory as is_constructible_v<unexpected<any>, expected<int, int>> is true. An LWG issue might be welcome but I currently have no idea on the resolution...

@StephanTLavavej StephanTLavavej merged commit d8ed02d into microsoft:main Sep 7, 2023
35 checks passed
@StephanTLavavej
Copy link
Member

Thanks for fixing this constraint recursion! 🔁 🛠️ 🎉

@achabense achabense deleted the _Fix_expected branch September 9, 2023 01:43
@achabense achabense changed the title Make expected<any, ...> and expected<void, any> copyable Avoid constraint recursion for expected<any, ...> and expected<void, any> Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
4 participants