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: wrapping traits #5175

Merged
merged 2 commits into from
Mar 3, 2024
Merged

Conversation

enitrat
Copy link
Contributor

@enitrat enitrat commented Feb 29, 2024

Added traits for wrapping arithmetic operations, based on the overflowing operations.
Removed reexporting from the ops module, to only have it at the num::traits level.


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


corelib/src/integer.cairo line 3013 at r1 (raw file):

/// WrappingAdd implementations
impl TWrappingAdd<T, +core::num::traits::OverflowingAdd<T>> of core::num::traits::WrappingAdd<T> {

for must types these implementations are less efficient than direct implementations currently.
the generic implementation will actually block efficient implementation - which is a problem.

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


corelib/src/integer.cairo line 3013 at r1 (raw file):

Previously, orizi wrote…

for must types these implementations are less efficient than direct implementations currently.
the generic implementation will actually block efficient implementation - which is a problem.

you may instead add these as an internal mod overflowing_based near the trait, and use it directly for all the reexported types here.

Copy link
Contributor Author

@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.

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


corelib/src/integer.cairo line 3013 at r1 (raw file):

the generic implementation will actually block efficient implementation

I'm not sure I understand: what's the problem with this approach and why would it make it less effician than adding these in an internal module if I reexport it with impl aliases?

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


corelib/src/integer.cairo line 3013 at r1 (raw file):

Previously, enitrat (Mathieu) wrote…

the generic implementation will actually block efficient implementation

I'm not sure I understand: what's the problem with this approach and why would it make it less effician than adding these in an internal module if I reexport it with impl aliases?

for example - for multiplication - the implementation would have just been wide mult + masking, but this already does more - and this implementation catches all, so cannot be improved.

may be ok just because it is within integer.cairo - but i still rather be more careful here.

Copy link
Contributor Author

@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.

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


corelib/src/integer.cairo line 3013 at r1 (raw file):

Previously, orizi wrote…

for example - for multiplication - the implementation would have just been wide mult + masking, but this already does more - and this implementation catches all, so cannot be improved.

may be ok just because it is within integer.cairo - but i still rather be more careful here.

Ok - got it. If I understand well I should rewrite them directly instead of being overflow_based, because there are extra unused return parameters adding unnecessary extra steps and we should aim for efficiency.

I can just write the full implementation directly instead of relying on Overflowing traits if that's better.

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

a discussion (no related file):
@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.

:lgtm:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @enitrat)

@gilbens-starkware gilbens-starkware added this pull request to the merge queue Mar 3, 2024
Merged via the queue into starkware-libs:main with commit a7da65c Mar 3, 2024
42 of 43 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.

3 participants