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

Separated integers from rationals and added simple FastInteger wrapper #751

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Tomaqa
Copy link
Member

@Tomaqa Tomaqa commented Aug 21, 2024

Existing FastRational class contained some functions that are not related to rationals but to integers.
I fixed this by making a derived class FastInteger and moving these specific functions there.

@Tomaqa Tomaqa requested a review from blishko August 21, 2024 19:05
@Tomaqa Tomaqa force-pushed the separate_integer branch 2 times, most recently from a02c3cc to 5569e3c Compare August 22, 2024 07:18
@Tomaqa
Copy link
Member Author

Tomaqa commented Aug 22, 2024

I also corrected arithmetic operations s.t. they return FastInteger and not FastRational.

@Tomaqa Tomaqa force-pushed the separate_integer branch 2 times, most recently from 64d1971 to dd14374 Compare August 23, 2024 14:42
@Tomaqa
Copy link
Member Author

Tomaqa commented Aug 23, 2024

I also experimented with a clearer distinction between Real and Integer, but it turned out to require quite a lot of work, so I gave up. The current solution partially divides the usage of the types, but it still uses integer operations on rational arguments while using static_cast on them. This is (currently) safe and the performance should be the same as before. But the current solution requires to explicitly cast/convert the arguments to integers, making it clearer what is going on.

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.

1 participant