-
Notifications
You must be signed in to change notification settings - Fork 357
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
Add Decimal{,256}::to_uint_floor and ::to_uint_ceil #1603
Conversation
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.
LGTM. There are two minor comments to consider if you want to. Obviously, they are also the same for 256 parts - I didn't want to repeat myself.
@@ -2002,6 +2050,42 @@ mod tests { | |||
)); | |||
} | |||
|
|||
#[test] | |||
fn decimal_to_uint_floor_works() { |
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.
Are those tests even needed? You have them in examples which makes them automatically tests (unless we disabled this behaviour for some reason? I don't think so). To me it is just a duplicate.
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.
Yeah, some are duplicates because I wrote the examples first. But I think it's good to have both to be able to optimize examples for teaching and tests for coverage long term.
You have them in examples which makes them automatically tests (unless we disabled this behaviour for some reason? I don't think so).
Yeah, doc tests are running as usual here
packages/std/src/math/decimal.rs
Outdated
if (self.0 % Self::DECIMAL_FRACTIONAL).is_zero() { | ||
self.0 / Self::DECIMAL_FRACTIONAL | ||
} else { | ||
self.0 / Self::DECIMAL_FRACTIONAL + Uint128::one() | ||
} |
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.
If we want to go for efficiency here, I think that this would go a bit better because of avoiding the fairly expensive modulo:
if self.0.is_zero() {
Self::zero()
} else {
(Self.0 - Uint128::one) / Self::DECIMAL_FRACTIONAL + Uint128::one()
}
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.
Good point, refactored
Right now people do
Uint128::one() * my_decimal
to perform the same operation, which is correct but hard to read and less efficient.