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: Default trait for Array/Dict #3048

Merged

Conversation

wraitii
Copy link
Contributor

@wraitii wraitii commented May 5, 2023

Fixes #3046

This does two things:

  • Leverage the existing Default trait instead of adding a custom method to ArrayTrait/Felt252DictTrait
  • Use that trait to create a simple new function that makes code leaner.

If the new function isn't desired, I'll remove it, but I think it's overall a good readability improvement.

NB: There is a new in BoxTrait that takes an argument. I haven't changed that.


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 35 files reviewed, 1 unresolved discussion (waiting on @wraitii)


corelib/src/lib.cairo line 11 at r1 (raw file):

fn new<T, impl TDefault: Default<T>>() -> T {
    Default::default()

name is "too useful" - i have no issues with the fn itself - but i think we need a bit of a longer name at the very least.
(and the fact that Default::default() would work seems to me to make it less necessary as well)

Copy link
Contributor

@spapinistarkware spapinistarkware 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 35 files at r1, all commit messages.
Reviewable status: 2 of 35 files reviewed, 1 unresolved discussion (waiting on @orizi)


corelib/src/lib.cairo line 11 at r1 (raw file):

Previously, orizi wrote…

name is "too useful" - i have no issues with the fn itself - but i think we need a bit of a longer name at the very least.
(and the fact that Default::default() would work seems to me to make it less necessary as well)

I don't like this function either. We can use Default::default()

@wraitii wraitii force-pushed the feat/default_construction branch from e5de055 to 804a2dd Compare May 8, 2023 12:33
@wraitii wraitii changed the title Feat: Default trait for Array/Dict and implement a new function Feat: Default trait for Array/Dict May 8, 2023
Copy link
Contributor Author

@wraitii wraitii 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: 2 of 35 files reviewed, 1 unresolved discussion (waiting on @orizi and @spapinistarkware)


corelib/src/lib.cairo line 11 at r1 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

I don't like this function either. We can use Default::default()

I've removed the function.
I think a shortened form could be useful (perhaps just default()), but that would be mostly sidestepped if we had something like vec![]

Copy link
Contributor

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 21 files at r2, all commit messages.
Reviewable status: 7 of 35 files reviewed, 2 unresolved discussions (waiting on @orizi and @wraitii)


corelib/src/debug.cairo line 24 at r2 (raw file):

fn print_felt252(message: felt252) {
    let mut arr = Default::default();

How does this actually work? With no type annotations, it can't know we want an array.

Copy link
Contributor Author

@wraitii wraitii 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: 7 of 35 files reviewed, 2 unresolved discussions (waiting on @orizi and @spapinistarkware)


corelib/src/debug.cairo line 24 at r2 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

How does this actually work? With no type annotations, it can't know we want an array.

I believe the type system deduces it from the print statement. I'm honestly not entirely sure about the type-deduction system in Cairo though.
But this compiles and runs without problem.

@wraitii wraitii requested review from spapinistarkware and orizi May 10, 2023 07:57
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 12 of 35 files at r1, 18 of 21 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: 35 of 36 files reviewed, 2 unresolved discussions (waiting on @spapinistarkware and @wraitii)


corelib/src/traits.cairo line 132 at r3 (raw file):

        @Default::default()
    }
}

Suggestion:

impl SnapshotDefault<T, impl TDefault: Default<T>, impl TDrop: Drop<T>> of Default<@T> {
    #[inline(always)]
    fn default() -> @T {
        @Default::default()
    }
}

Copy link
Contributor Author

@wraitii wraitii 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: 35 of 36 files reviewed, 2 unresolved discussions (waiting on @orizi and @spapinistarkware)


corelib/src/traits.cairo line 132 at r3 (raw file):

        @Default::default()
    }
}

Done

@wraitii wraitii force-pushed the feat/default_construction branch from 045293b to 3d7c914 Compare May 17, 2023 14:32
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 35 files at r1, 8 of 8 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)

Copy link
Contributor

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

@spapinistarkware
Copy link
Contributor

Looks like there are conflicts. If you resolve them, I'll merge

@wraitii wraitii force-pushed the feat/default_construction branch from 3d7c914 to f1894e5 Compare May 22, 2023 09:24
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 16 of 16 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @wraitii)

@wraitii wraitii force-pushed the feat/default_construction branch from f1894e5 to 074010f Compare May 24, 2023 09:18
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 5 of 5 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @wraitii)

@spapinistarkware spapinistarkware added this pull request to the merge queue May 24, 2023
Merged via the queue into starkware-libs:main with commit 62d9433 May 24, 2023
@wraitii wraitii deleted the feat/default_construction branch May 25, 2023 16:06
@lambda-0x
Copy link
Contributor

lambda-0x commented Jun 4, 2023

wouldn't it be better to have both ArrayTrait::new() and Default::default() because when default is used and type is not annotated manually it, reader would need to infer the type by looking at the variable usage.

And I like being more explicit way more. similar to rust where there is default but people still prefer to use new methods defined on type over default.

Not to mention this would be a breaking change for anyone using new methods.

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: Default trait
4 participants