Implement is_one and Inv for Ratio. #19
Conversation
clarfonthey
changed the title
src/lib.rs
Outdated
| @@ -715,6 +715,28 @@ impl<'a, T> Neg for &'a Ratio<T> | |||
| } | |||
| } | |||
|
|
|||
| impl<T> Inv for Ratio<T> | |||
| where T: Clone + Integer + Inv<Output = T> | |||
There was a problem hiding this comment.
We don't need T: Inv -- in fact I expect no integers would have it.
src/lib.rs
Outdated
| type Output = Ratio<T>; | ||
|
|
||
| #[inline] | ||
| fn neg(self) -> Ratio<T> { |
|
Will work once |
|
|
||
| #[inline] | ||
| fn is_one(&self) -> bool { | ||
| self.numer == self.denom |
There was a problem hiding this comment.
Should we consider the degenerate 0/0 case?
There was a problem hiding this comment.
I mean, considering how 0/0 is an invalid state for Ratio, I'd assume that it shouldn't be worried about.
There was a problem hiding this comment.
That's fair, and is_zero doesn't worry about it either.
There was a problem hiding this comment.
Thinking about it, I'll add some debug_asserts to verify that the denominator is nonzero so that these can be caught in debug mode, but won't be checked on release.
I'll do this in a separate PR.
Cargo.toml
Outdated
| @@ -16,15 +16,15 @@ readme = "README.md" | |||
|
|
|||
| [dependencies.num-bigint] | |||
| optional = true | |||
| version = "0.2.0-git" | |||
| version = "0.2.1" | |||
There was a problem hiding this comment.
Why is this needed?
num-bigint, complex, and rational are all currently in a transition to 0.2, and haven't been published at all yet -- thus the git dependency. But I don't see why this PR would depend on bigint anyway.
There was a problem hiding this comment.
(If you want your new num-traits implementations in these traits sooner, please follow up with PRs to their 0.1.x branches.)
There was a problem hiding this comment.
It wasn't compiling for me and I was a bit confused, and then I realised that I had to run cargo update. ><
|
Okay, everything should be working now. I relaxed some of the bounds on the methods and hopefully that should be okay; please take a look at the latest commit to make sure everything is reasonable. |
|
Are the changes to bounds actually useful? If you can't even call (In general, I'd prefer you didn't combine unrelated changes into one PR.) |
|
@cuviper will remove those then |
|
Reverted those changes, so, things should be ready to merge. |
|
Thanks! bors r+ |
Build succeeded |
Requires a new version of
num-traitsto be pushed but I figured I'd offer this anyway.