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

wrapping add #9

Merged
merged 11 commits into from
Sep 25, 2023
Merged

wrapping add #9

merged 11 commits into from
Sep 25, 2023

Conversation

Easyoakland
Copy link
Contributor

Adding methods for wrapping.

I don't know if there is a better way to implement these.

Currently only wrapping_add is implemented but I can work on the others if this is the correct way to go about it.

@jhpratt
Copy link
Owner

jhpratt commented Sep 1, 2023

I'm surprised you managed to figure out the inner workings, as basically nothing is documented right now.

Wrapping, checked, and overflowing arithmetic is something that I wanted to implement for a while, and was thinking about recently.

I will take a look into this soon! I imagine it'll take some thinking on my end.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Panics if range_size greater than inner::MAX.
This only applies on signed integers.
There might be a way to read bytes as unsigned, wrap add, then read bytes as signed.
Using something like to_le_bytes and from_le_bytes.
@Easyoakland
Copy link
Contributor Author

Easyoakland commented Sep 6, 2023

The implementation panics if the number is signed and the range is too large. Ideally this could be fixed with methods like checked_add_unsigned but the if_signed macro doesn't seem to work inside the method body and the unsigned methods are not implemented for unsigned types.

Although there isn't an equivalent rem_euclid for unsigned arguments so maybe that technique won't work anyway.

@jhpratt
Copy link
Owner

jhpratt commented Sep 6, 2023

checked_add_unsigned should work. It'll require the MSRV be bumped, but that's not an issue.

As to rem_euclid, I imagine it's possible to implement it manually. I would provide a function definition, but I don't have sufficient time at the moment.

Still clarifying/checking wrapping add method works.

It seems to work for all input I can come up with but since I haven't
finished verifying it might be wrong.
- It was actually much simpler than previously realized.
- The tests might be overkill
@Easyoakland
Copy link
Contributor Author

@jhpratt I think I finished the no_panic implementation with a valid explanation why the algorithm works.
Feel free to check that it's actually valid.

@jhpratt
Copy link
Owner

jhpratt commented Sep 7, 2023

Thanks, I'll check it out! Now you know why I was putting off implementing it myself 😅

@jhpratt
Copy link
Owner

jhpratt commented Sep 24, 2023

Quick status update: Apologies for the delay! Between RustConf and some other things, I've been quite busy. I intend to look at this in the next day or two.

Copy link
Owner

@jhpratt jhpratt left a comment

Choose a reason for hiding this comment

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

Implementation looks correct. I only have one question and two nits.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/tests.rs Outdated Show resolved Hide resolved
@jhpratt
Copy link
Owner

jhpratt commented Sep 25, 2023

Thanks for the changes! Everything LGTM. Merging.

@jhpratt jhpratt merged commit 13bed1e into jhpratt:main Sep 25, 2023
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.

2 participants