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

Integers not correctly handled in num::rational #7

Open
cuviper opened this issue Dec 19, 2017 · 1 comment
Open

Integers not correctly handled in num::rational #7

cuviper opened this issue Dec 19, 2017 · 1 comment

Comments

@cuviper
Copy link
Member

cuviper commented Dec 19, 2017

From @rust-highfive on September 28, 2014 22:43

Issue by tbu-
Saturday Aug 09, 2014 at 13:25 GMT

For earlier discussion, see rust-lang/rust#16386

This issue was labelled with: in the Rust repository


  • Possible-machine integers are treated as mathematical integers: Ratio::reduce, where the unary minus operator on a negative number is assumed to yield a positive number.
  • self.denom is compared to Zero::zero instead of calling is_zero().
  • Ratio::recip might return a fractions with a denominator of 0.

There are also some issues regarding overflows, it seems the module just hopes that they won't happen at all – except for wrong results, this could also raise a fail!()ure, when e.g. 1/sqrt(max_int) is multiplied with itself, yielding 1/0.

This might mean that Rust needs some general guideline on how to treat integer overflow.

Copied from original issue: rust-num/num#11

@vks
Copy link
Contributor

vks commented Feb 10, 2021

self.denom is compared to Zero::zero instead of calling is_zero().

This is fixed.

This might mean that Rust needs some general guideline on how to treat integer overflow.

I think we should adopt the same behavior as for the primitive integer types: Panic in debug mode, silently fail in release mode. This would mean that this issue and #11 can be just closed, because all remaining cases already panic in debug mode.

We could add debug_asserts, but I don't think it adds much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants