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

Probably not something you want... #5

Open
wants to merge 75 commits into
base: master
Choose a base branch
from
Open

Conversation

mredig
Copy link

@mredig mredig commented Oct 26, 2024

But I'll let you be the judge of that!

This isn't really an improvement so much as achieving different goals than what you had here already. I wanted to be able to use this without worrying about buffer overflows and incorporated BigInt into the featureset, replacing support for generic numeric values.

I also refactored the automatic reduction of values (2/4 -> 1/2) to instead be a property on an instance (or with an optional initializer flag) as I have a use case for not immediately reducing the fraction.

I honestly do not expect you want to merge seeing as this is, once again, achieving different goals than previously.

In the event that I'm incorrect and you actually do want these changes, just be aware that I still have some cleanup to do and can revert some of the readme stuff (like the name change), so please just let me know :)

mredig added 27 commits October 25, 2024 16:08
This fundamentally changes much of the functionality around edge cases, since buffer overflows are no longer an issue.
updated with new functionality
updated with new functionality
updated with new functionality
removed irrelevant edge case tests
@abdel-17
Copy link
Owner

Hey! I have actually been considering adding a BigInt equivalent, but there was no native implementation in Swift.

I honestly much prefer having everything be normalized as that enables some optimizations under the hood, but overflows are definitely a problem I want to solve somehow. I remember there were multiple BigInt implementations for Swift last time I checked. Why did you pick this one specifically?

@mredig
Copy link
Author

mredig commented Oct 27, 2024

Hey! I have actually been considering adding a BigInt equivalent, but there was no native implementation in Swift.

🔥

I honestly much prefer having everything be normalized as that enables some optimizations under the hood, but overflows are definitely a problem I want to solve somehow.

Well I definitely need normalization as an option for my use, so that might just be a non starter when it comes to merging back in. However, if there are other bits of the code you want, I would be more than happy for you to grab them.

Out of curiosity, what is your use case that needs that level of uber optimization? Personally, I'm creating a calculator where having an unreduced fraction might be a legitimate step in breaking down an equation. That's up to the user. It also means that extreme optimization isn't really necessary since each calculation will be separated by some user input. No loops or fast and frequent calculations requiring that level of tight optimizations.

I remember there were multiple BigInt implementations for Swift last time I checked. Why did you pick this one specifically?

Honestly, I didn't put that much thought into it. It was mostly a gut feeling, however, I remember looking at the project and thinking it was well documented, that the code looked good to me, and it appeared to be mature with relatively good support. I DID look at several different projects, both BigInt and Rational in nature and ultimately felt like I liked the developer experience of these two best. Ymmv, of course!

@mredig
Copy link
Author

mredig commented Oct 27, 2024

(it might delight you to know that I actually came across this BigInt from another Rational library, but didn't like theirs as much as yours, hence taking the time to combine them :) )

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