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

feat(corelib): add boolean operators to Result type #6921

Merged

Conversation

cairolover
Copy link
Contributor

Adds and, and_then, or, or_else, unwrap_or_else to ResultTrait along with tests.

note: the module documentation is taken from Rust's. the item-level doc was adapted to Cairo.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

@cairolover cairolover left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion


corelib/src/result.cairo line 371 at r1 (raw file):

    /// assert!(x.or(y) == Result::Ok(2));
    /// ```
    //TODO: why can't I restrict E and U to implement Destruct rather than Drop?

Same comment for and: I'm not sure why this can't be +Destruct<T>, +Destruct<F>, +Destruct<E> rather than +Drop<T>, +Drop<F>, +Destruct<E>

@cairolover cairolover changed the title feat: add boolean operators to Result type feat(corelib0: add boolean operators to Result type Dec 24, 2024
@cairolover cairolover changed the title feat(corelib0: add boolean operators to Result type feat(corelib): add boolean operators to Result type Dec 24, 2024
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @cairolover)


a discussion (no related file):
2nd eye @TomerStarkware


corelib/src/result.cairo line 84 at r1 (raw file):

//! [`Ok`]: Result::Ok
//! [`Err`]: Result::Err
//!

don't move the comment block.


corelib/src/result.cairo line 317 at r1 (raw file):

            Result::Err(e) => Result::Err(e),
        }
    }

Suggestion:

    #[inline]
    fn and<U, +Destruct<T>, +Drop<E>, +Drop<U>>(
        self: Result<T, E>, other: Result<U, E>,
    ) -> Result<U, E> {
        match self {
            Result::Ok(_) => other,
            Result::Err(e) => Result::Err(e),
        }
    }

corelib/src/result.cairo line 380 at r1 (raw file):

            Result::Err(_) => res,
        }
    }

Suggestion:

    #[inline]
    fn or<F, +Drop<T>, +Drop<F>, +Destruct<E>>(
        self: Result<T, E>, other: Result<T, F>,
    ) -> Result<T, F> {
        match self {
            Result::Ok(v) => Result::Ok(v),
            Result::Err(_) => other,
        }
    }

Copy link
Collaborator

@TomerStarkware TomerStarkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @cairolover and @orizi)

Copy link
Contributor Author

@cairolover cairolover left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @orizi)


corelib/src/result.cairo line 371 at r1 (raw file):

Previously, cairolover (cairolover) wrote…

Same comment for and: I'm not sure why this can't be +Destruct<T>, +Destruct<F>, +Destruct<E> rather than +Drop<T>, +Drop<F>, +Destruct<E>

still unsure about that; but if it's fine for you, then I removed the comment.


corelib/src/result.cairo line 317 at r1 (raw file):

            Result::Err(e) => Result::Err(e),
        }
    }

Done.


corelib/src/result.cairo line 380 at r1 (raw file):

            Result::Err(_) => res,
        }
    }

Done.

Copy link
Contributor Author

@cairolover cairolover left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @orizi)


corelib/src/result.cairo line 84 at r1 (raw file):

Previously, orizi wrote…

don't move the comment block.

Method overview should come after imo. otherwise the ? operator description is burried under the methods description

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @cairolover)


corelib/src/result.cairo line 84 at r1 (raw file):

Previously, cairolover (cairolover) wrote…

Method overview should come after imo. otherwise the ? operator description is burried under the methods description

maybe - but this is completely unrelated to the PR.

Copy link
Contributor Author

@cairolover cairolover left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @orizi)


corelib/src/result.cairo line 84 at r1 (raw file):

Previously, orizi wrote…

maybe - but this is completely unrelated to the PR.

Done.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @cairolover)


a discussion (no related file):
@enitrat for doc 2nd eye.

Copy link
Contributor

@enitrat enitrat left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @cairolover and @orizi)

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r1, 1 of 1 files at r4.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @cairolover)


corelib/src/result.cairo line 277 at r4 (raw file):

        +Destruct<E>,
        +Drop<F>,
        impl func: core::ops::FnOnce<F, (E,)>[Output: T],

Suggestion:

        +core::ops::FnOnce<F, (E,)>[Output: T],

corelib/src/result.cairo line 340 at r4 (raw file):

    /// ```
    #[inline]
    fn and_then<U, F, +Drop<F>, impl func: core::ops::FnOnce<F, (T,)>[Output: Result<U, E>]>(

Suggestion:

    fn and_then<U, F, +Drop<F>, +core::ops::FnOnce<F, (T,)>[Output: Result<U, E>]>(

corelib/src/result.cairo line 400 at r4 (raw file):

    /// ```
    #[inline]
    fn or_else<F, O, +Drop<O>, impl func: core::ops::FnOnce<O, (E,)>[Output: Result<T, F>]>(

Suggestion:

    fn or_else<F, O, +Drop<O>, +func: core::ops::FnOnce<O, (E,)>[Output: Result<T, F>]>(

Copy link
Contributor Author

@cairolover cairolover left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @orizi)


corelib/src/result.cairo line 277 at r4 (raw file):

        +Destruct<E>,
        +Drop<F>,
        impl func: core::ops::FnOnce<F, (E,)>[Output: T],

Done. (removed restriction on func::Output as well)


corelib/src/result.cairo line 340 at r4 (raw file):

    /// ```
    #[inline]
    fn and_then<U, F, +Drop<F>, impl func: core::ops::FnOnce<F, (T,)>[Output: Result<U, E>]>(

Done.


corelib/src/result.cairo line 400 at r4 (raw file):

    /// ```
    #[inline]
    fn or_else<F, O, +Drop<O>, impl func: core::ops::FnOnce<O, (E,)>[Output: Result<T, F>]>(

Done.

@orizi orizi enabled auto-merge December 26, 2024 08:03
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion

@orizi orizi added this pull request to the merge queue Dec 26, 2024
Merged via the queue into starkware-libs:main with commit 1146131 Dec 26, 2024
46 of 47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants