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

Fix FastRational and test it s.t. it is reliable #713

Open
Tomaqa opened this issue May 16, 2024 · 1 comment
Open

Fix FastRational and test it s.t. it is reliable #713

Tomaqa opened this issue May 16, 2024 · 1 comment
Labels
investigate Suspicious or improper behaviour that needs to be investigates priority:medium Medium (normal) priority issue

Comments

@Tomaqa
Copy link
Member

Tomaqa commented May 16, 2024

Currently, FastRational uses signed integer for numerator and unsigned for denominator. This yields conversion issues and mixing such variables is not recommended in C++. I would be probably better to stick to just signed integers (e.g. as in boost::rational) and detect whether they overflow after arithmetic operations.
It can be also worth trying whether using boost::rational directly (if the values fit into 32 bits) would not be more reliable and also more efficient.
Supporting the class with tests would be good.

#712 identifies a bug in constructor caused by incorrect casting.

@Tomaqa Tomaqa added investigate Suspicious or improper behaviour that needs to be investigates priority:medium Medium (normal) priority issue labels May 16, 2024
@blishko
Copy link
Member

blishko commented May 16, 2024

I believe the only reason for using unsigned type for denumerator is to extend the range of values for which we can use the fast representation.
But maybe it won't make much difference and could simplify the code.

I am against adding a dependency on boost unless there is a clear benefit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
investigate Suspicious or improper behaviour that needs to be investigates priority:medium Medium (normal) priority issue
Projects
None yet
Development

No branches or pull requests

2 participants