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 P2505R5 Monadic Functions For std::expected #3361

Merged
merged 54 commits into from
Feb 10, 2023

Conversation

TartanLlama
Copy link
Member

@TartanLlama TartanLlama commented Jan 25, 2023

Implements P2505R5 Monadic Functions for std::expected
Fixes #3210

Open question: The transform and transform_error specifications have Mandates clauses similar to:

Mandates: U is a valid value type for expected. If is_void_v<U> is false, the declaration U u(invoke(std::forward<F>(f), value())); is well-formed.

Do we want static_asserts for these?

@TartanLlama TartanLlama marked this pull request as ready for review January 25, 2023 14:48
@TartanLlama TartanLlama requested a review from a team as a code owner January 25, 2023 14:48
Copy link
Contributor

@AlexGuteniev AlexGuteniev left a comment

Choose a reason for hiding this comment

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

One question from me (regarding all occurrences, not just this one)

stl/inc/expected Outdated Show resolved Hide resolved
@miscco

This comment was marked as resolved.

@AlexGuteniev

This comment was marked as resolved.

@CaseyCarter CaseyCarter added the cxx23 C++23 feature label Jan 25, 2023
@cpplearner
Copy link
Contributor

At least we could extract the common parts into a helper function template. Currently the definition of transform takes 72 lines in the primary class template, and will only take 42 lines if the function bodies are shared.

the 42-line implementation that I was thinking of
    template <class _SelfType, class _Fn>
    static constexpr auto _Transform_expected(_SelfType&& _Self, _Fn&& _Func) {
        _STL_INTERNAL_STATIC_ASSERT(is_same_v<remove_cvref_t<_SelfType>, expected>);

        using _Uty = remove_cv_t<invoke_result_t<_Fn, decltype(*_STD declval<_SelfType>())>>;

        if (_Self._Has_value) {
            if constexpr (is_void_v<_Uty>) {
                _STD invoke(_STD forward<_Fn>(_Func), _STD forward_like<_SelfType>(_Self._Value));
                return expected<_Uty, _Err>();
            } else {
                return expected<_Uty, _Err>(_Construct_expected_from_invoke_result_tag{},
                    _STD forward<_Fn>(_Func), _STD forward_like<_SelfType>(_Self._Value));
            }
        } else {
            return expected<_Uty, _Err>(unexpect, _STD forward_like<_SelfType>(_Self._Unexpected));
        }
    }

    template <class _Fn>
        requires is_copy_constructible_v<_Err>
    constexpr auto transform(_Fn&& _Func) & {
        return _Transform_expected(*this, _STD forward<_Fn>(_Func));
    }

    template <class _Fn>
        requires is_copy_constructible_v<_Err>
    constexpr auto transform(_Fn&& _Func) const& {
        return _Transform_expected(*this, _STD forward<_Fn>(_Func));
    }

    template <class _Fn>
        requires is_move_constructible_v<_Err>
    constexpr auto transform(_Fn&& _Func) && {
        return _Transform_expected(_STD move(*this), _STD forward<_Fn>(_Func));
    }

    template <class _Fn>
        requires is_move_constructible_v<_Err>
    constexpr auto transform(_Fn&& _Func) const&& {
        return _Transform_expected(_STD move(*this), _STD forward<_Fn>(_Func));
    }

Copy link
Contributor

@AlexGuteniev AlexGuteniev left a comment

Choose a reason for hiding this comment

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

Some nitpicking

@strega-nil-ms

This comment was marked as resolved.

@microsoft microsoft deleted a comment from azure-pipelines bot Jan 25, 2023
@azure-pipelines

This comment was marked as resolved.

@AlexGuteniev

This comment was marked as resolved.

@StephanTLavavej

This comment was marked as resolved.

stl/inc/expected 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
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! Note to self: I reviewed the product code up to the void specialization and still need to review the new test file.

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 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
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
stl/inc/expected Outdated Show resolved Hide resolved
stl/inc/expected Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

Thanks! I finished reviewing and pushed changes for the remaining issues I found (the most notable being missing mandate enforcement), followed by a conflict-free merge with main. With that I think we're ready to go! 😸

@StephanTLavavej StephanTLavavej removed their assignment Feb 10, 2023
stl/inc/expected Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej self-assigned this Feb 10, 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 8f91285 into microsoft:main Feb 10, 2023
@StephanTLavavej
Copy link
Member

Thanks for implementing this C++23 feature - it's expected to be an awesome Standard! 😹 😻 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx23 C++23 feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

P2505R5 Monadic Functions For expected
9 participants