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

Add cmp::minmax function #4981

Merged
merged 7 commits into from
Feb 6, 2024
Merged

Conversation

delaaxe
Copy link
Contributor

@delaaxe delaaxe commented Feb 2, 2024

Will add rest of tests if this is accepted


This change is Reviewable

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, 3 unresolved discussions (waiting on @delaaxe)


corelib/src/cmp.cairo line 18 at r1 (raw file):

#[must_use]
pub fn min_max<T, +PartialOrd<T>, +Drop<T>, +Copy<T>>(a: T, b: T) -> (T, T) {

add doc


corelib/src/cmp.cairo line 23 at r1 (raw file):

    }
    (a, b)
}

Suggestion:

#[must_use]
pub fn minmax<T, +PartialOrd<T>, +Drop<T>, +Copy<T>>(a: T, b: T) -> (T, T) {
    if a > b {
        (b, a)
    } else {
        (a, b)
    }
}

corelib/src/test/cmp_test.cairo line 46 at r1 (raw file):

    assert_eq(@min_max(120_u8, 130_u8), @(120_u8, 130_u8), '130 > 120');
    assert_eq(@min_max(200_u8, 150_u8), @(150_u8, 200_u8), '200 > 150');
}

Suggestion:

#[test]
fn test_min_max_u8() {
    assert!(minmax(0_u8, 1_u8) == (0_u8, 1_u8));
    assert!(minmax(5_u8, 7_u8) == (5_u8, 7_u8));
    assert!(minmax(255_u8, 128_u8) == (128_u8, 255_u8));
    assert!(minmax(10_u8, 10_u8) == (10_u8, 10_u8));
    assert!(minmax(0_u8, 0_u8) == (0_u8, 0_u8));
    assert!(minmax(255_u8, 255_u8) == (255_u8, 255_u8));
    assert!(minmax(100_u8, 200_u8) == (100_u8, 200_u8));
    assert!(minmax(1_u8, 2_u8) == (1_u8, 2_u8));
    assert!(minmax(120_u8, 130_u8) == (120_u8, 130_u8));
    assert!(minmax(200_u8, 150_u8) == (150_u8, 200_u8));
}

@delaaxe
Copy link
Contributor Author

delaaxe commented Feb 5, 2024

Removed @ signs but not sure it actually compiles locally, can you approve the workflows? if/else suggestion doesn't match style of functions just above

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 @delaaxe)


corelib/src/test/cmp_test.cairo line 46 at r1 (raw file):

    assert_eq(@min_max(120_u8, 130_u8), @(120_u8, 130_u8), '130 > 120');
    assert_eq(@min_max(200_u8, 150_u8), @(150_u8, 200_u8), '200 > 150');
}

Look at the suggestion exactly, you did something else.
Also, use reviewable so we'd follow the same threads.

@delaaxe
Copy link
Contributor Author

delaaxe commented Feb 5, 2024

Oh indeed, fixed. Which file should be edited for documentation?

@delaaxe delaaxe changed the title Add cmp::min_max function Add cmp::minmax function Feb 5, 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.

use reviewable to answer in thread.

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

Copy link
Contributor Author

@delaaxe delaaxe 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, 3 unresolved discussions (waiting on @orizi)


corelib/src/cmp.cairo line 18 at r1 (raw file):

Previously, orizi wrote…

add doc

can you point to the file pls?


corelib/src/cmp.cairo line 23 at r1 (raw file):

    }
    (a, b)
}

doesn't match style of functions above, should I change them all?


corelib/src/test/cmp_test.cairo line 46 at r1 (raw file):

Previously, orizi wrote…

Look at the suggestion exactly, you did something else.
Also, use reviewable so we'd follow the same threads.

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 2 of 2 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @delaaxe)


corelib/src/cmp.cairo line 18 at r1 (raw file):

Previously, delaaxe wrote…

can you point to the file pls?

just here - i know - the other functions don't have proper doc - but that's a good place to start.


corelib/src/cmp.cairo line 23 at r1 (raw file):

Previously, delaaxe wrote…

doesn't match style of functions above, should I change them all?

yeah - sure.

Copy link
Contributor Author

@delaaxe delaaxe 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: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @orizi)


corelib/src/cmp.cairo line 18 at r1 (raw file):

Previously, orizi wrote…

just here - i know - the other functions don't have proper doc - but that's a good place to start.

Done.


corelib/src/cmp.cairo line 23 at r1 (raw file):

Previously, orizi wrote…

yeah - sure.

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.

:lgtm:

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @delaaxe)

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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @delaaxe)

a discussion (no related file):
adding @gilbens-starkware for 2nd eye.


Copy link
Contributor

@gilbens-starkware gilbens-starkware 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: all files reviewed, 1 unresolved discussion (waiting on @delaaxe)


corelib/src/cmp.cairo line 12 at r4 (raw file):

    } else {
        a
    }

Suggestion:

/// # Arguments
/// * `a` - first comparable value
/// * `b` - Second comparable value
/// # Returns
/// * `result` - The smallest of the two values
#[must_use]
pub fn min<T, +PartialOrd<T>, +Drop<T>, +Copy<T>>(a: T, b: T) -> T {
    if a > b {
        b
    } else {
        a
    }

Copy link
Contributor

@gilbens-starkware gilbens-starkware 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 r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @delaaxe)


corelib/src/cmp.cairo line 30 at r4 (raw file):

}

/// Minumum and maximum of the two values.

Suggestion:

/// Minimum a

Copy link
Contributor Author

@delaaxe delaaxe left a comment

Choose a reason for hiding this comment

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

Good catches, fixed!

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware and @orizi)


corelib/src/cmp.cairo line 12 at r4 (raw file):

    } else {
        a
    }

Done.


corelib/src/cmp.cairo line 30 at r4 (raw file):

}

/// Minumum and maximum of the two values.

Done.

Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 2 files at r3, 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @delaaxe)

@gilbens-starkware gilbens-starkware added this pull request to the merge queue Feb 6, 2024
Merged via the queue into starkware-libs:main with commit 60340c8 Feb 6, 2024
43 checks passed
@delaaxe delaaxe deleted the feat/min-max branch February 9, 2024 15:40
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.

3 participants