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

Implement OpAssign for Ratio #12

Merged
merged 1 commit into from
Jan 26, 2018
Merged

Implement OpAssign for Ratio #12

merged 1 commit into from
Jan 26, 2018

Conversation

c410-f3r
Copy link
Contributor

Copied shamelessly from #274 but fixes #5.

There is only one problem to solve before anything. Since the implementation doesn't use the reduce function, it is possible to exist multiple types of zeros (0/4, 0/21, 0/66) instead of the default 0/1, which implies in test errors like test(_1_2, _NEG1_2, _0);, therefore, should I rewrite the test suite to be more "zero" friendly?

@cuviper
Copy link
Member

cuviper commented Jan 19, 2018

Since the implementation doesn't use the reduce function [...]

I think it should though, because un-reduced ratios make overflow (#13) even worse. To call reduce these will need an Integer bound, but that's fine because practically everything in Ratio does. Other than this, the PR looks good to me.

I notice that you added integer support, OpAssign<T> -- this is fine, but we don't even have the regular ops like this! Can I trouble you to try adding those too?

@c410-f3r
Copy link
Contributor Author

Thanks! Although it costs a few more instructions, reduce makes things much easier.

@cuviper
Copy link
Member

cuviper commented Jan 24, 2018

Can you merge or rebase your branch? I think your changes are colliding with #11.

src/lib.rs Outdated
@@ -1253,17 +1449,6 @@ mod test {
fn test_div_0() {
let _a = _1 / _0;
}

#[test]
fn test_checked_failures() {
Copy link
Member

Choose a reason for hiding this comment

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

It seems this test was accidentally removed in your merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow! That was bad of me :(

@cuviper
Copy link
Member

cuviper commented Jan 25, 2018

Now you pulled in a src/lib.rs.orig -- I suggest amending that commit and force-push it, so it doesn't pollute the history.

(If you have trouble with this, I may be able to push such an update to your PR myself.)

@c410-f3r
Copy link
Contributor Author

I rewrote history to make things cleaner.

@cuviper
Copy link
Member

cuviper commented Jan 26, 2018

Great, thanks!

bors r+

bors bot added a commit that referenced this pull request Jan 26, 2018
12: Implement OpAssign for Ratio r=cuviper a=c410-f3r

Copied shamelessly from [#274](rust-num/num#274) but fixes #5.

There is only one problem to solve before anything. Since the implementation doesn't use the `reduce` function, it is possible to exist multiple types of zeros (`0/4`, `0/21`, `0/66`) instead of the default `0/1`, which implies in test errors like `test(_1_2, _NEG1_2, _0);`, therefore, should I rewrite the test suite to be more "zero"  friendly?
@bors
Copy link
Contributor

bors bot commented Jan 26, 2018

Build succeeded

@bors bors bot merged commit f1091aa into rust-num:master Jan 26, 2018
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.

Support the assignment operators
2 participants