-
Notifications
You must be signed in to change notification settings - Fork 97
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 Saturating #43
Conversation
I've implemented some tests, so this is probably ready for a review (whenever you get a chance, of course!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass looks pretty good. Change commit message to "Implement num::Saturating
\n\nPart of #35." The standard library also has saturating_mul
which num::Saturating
doesn't.
src/system.rs
Outdated
where | ||
D: Dimension + ?Sized, | ||
U: Units<V> + ?Sized, | ||
V: $crate::num::Saturating + $crate::num::Num + $crate::Conversion<V>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move Saturating
after Conversion<V>
.
src/system.rs
Outdated
{ | ||
fn saturating_add(self, v: Self) -> Self { | ||
Quantity {value: self.value.saturating_add(v.value), ..self} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put a blank link after the closing }
.
src/system.rs
Outdated
V: $crate::num::Saturating + $crate::num::Num + $crate::Conversion<V>, | ||
{ | ||
fn saturating_add(self, v: Self) -> Self { | ||
Quantity {value: self.value.saturating_add(v.value), ..self} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space before value
. Space after ..self
.
src/system.rs
Outdated
Quantity {value: self.value.saturating_add(v.value), ..self} | ||
} | ||
fn saturating_sub(self, v: Self) -> Self { | ||
Quantity {value: self.value.saturating_sub(v.value), ..self} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space before value
. Space after ..self
.
Could you also add details to |
Thanks for the comments! I've addressed them and updated the branch. |
Could you move |
Done! Thanks for the reviews :) |
Looks like Appveyor found a pre-existing bug. Filed #48. |
Thanks! |
Part of #35.
While in general the test suite does not pass with integers enabled, the new tests do pass, and no new failures were introduced.