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

Added match_nullable_snapshot. #4253

Merged
merged 1 commit into from
Oct 17, 2023
Merged

Added match_nullable_snapshot. #4253

merged 1 commit into from
Oct 17, 2023

Conversation

orizi
Copy link
Collaborator

@orizi orizi commented Oct 12, 2023

This change is Reviewable

@orizi orizi requested a review from yuvalsw October 12, 2023 17:39
Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @orizi)


-- commits line 2 at r1:
Why is it needed?


corelib/src/nullable.cairo line 35 at r1 (raw file):

        nullable_from_box(BoxTrait::new(value))
    }
    fn is_null(self: @Nullable<T>) -> bool {

please also test is_null.


crates/cairo-lang-sierra-to-casm/src/invocations/nullable.rs line 27 at r1 (raw file):

        NullableConcreteLibfunc::MatchNullable(_)
        | NullableConcreteLibfunc::MatchNullableSnapshot(_) => {
            build_nullable_match_nullable(builder)

If it's the exact same casm, why do we need another libfunc?


tests/e2e_test_data/libfuncs/nullable line 121 at r1 (raw file):


//! > cairo
fn foo(x: @Nullable::<Array<felt252>>, y: Box::<@Array<felt252>>) -> Box::<@Array<felt252>> {

remove all "::"

Code quote:

able::<Array<felt252>>, y: Box::<@Array<felt252>>) -> Box::<@Array<felt252>>

@orizi orizi force-pushed the orizi/match-nullable-snapshot branch from c083956 to a91c6e9 Compare October 15, 2023 16:45
Copy link
Collaborator Author

@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: 6 of 9 files reviewed, 3 unresolved discussions (waiting on @yuvalsw)


-- commits line 2 at r1:

Previously, yuvalsw wrote…

Why is it needed?

specifically for the non-consuming is_null.


corelib/src/nullable.cairo line 35 at r1 (raw file):

Previously, yuvalsw wrote…

please also test is_null.

Done.


crates/cairo-lang-sierra-to-casm/src/invocations/nullable.rs line 27 at r1 (raw file):

Previously, yuvalsw wrote…

If it's the exact same casm, why do we need another libfunc?

it is a different signature - same code - as always with snapshots.

@orizi orizi force-pushed the orizi/match-nullable-snapshot branch from a91c6e9 to 3b68660 Compare October 15, 2023 16:47
Copy link
Contributor

@yuvalsw yuvalsw 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 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @orizi)


corelib/src/test/nullable_test.cairo line 8 at r2 (raw file):

    let nullable_x = NullableTrait::new(x);
    assert(!nullable_x.is_null(), 'nullable_x.is_null() true');
    assert_eq(@nullable_x.deref(), @x, 'x != 10');

x is 10 anyway, rephrase?

Code quote:

x != 10

corelib/src/test/nullable_test.cairo line 17 at r2 (raw file):

}

// Test objects of size>1.

rename the function accordingly?

Code quote:

// Test objects of size>1.

@orizi orizi force-pushed the orizi/match-nullable-snapshot branch from 3b68660 to 4e2c005 Compare October 16, 2023 14:15
Copy link
Collaborator Author

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


corelib/src/test/nullable_test.cairo line 8 at r2 (raw file):

Previously, yuvalsw wrote…

x is 10 anyway, rephrase?

Done.


corelib/src/test/nullable_test.cairo line 17 at r2 (raw file):

Previously, yuvalsw wrote…

rename the function accordingly?

Done.

@orizi orizi linked an issue Oct 16, 2023 that may be closed by this pull request
1 task
Copy link
Contributor

@yuvalsw yuvalsw 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 r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @orizi)


corelib/src/test/nullable_test.cairo line 17 at r3 (raw file):

}

// Testing `u256` as a tests for objects of size larger than 1.

Suggestion:

s a test fo

@orizi orizi force-pushed the orizi/match-nullable-snapshot branch from 4e2c005 to 73fc773 Compare October 17, 2023 06:52
Copy link
Collaborator Author

@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 r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @orizi)

@orizi orizi added this pull request to the merge queue Oct 17, 2023
Merged via the queue into main with commit 33fa8f5 Oct 17, 2023
@orizi orizi deleted the orizi/match-nullable-snapshot branch November 6, 2023 07:03
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.

feat: is_null function in NullableTrait
2 participants