Create pub constant Decimal::MAX_SCALE #685
Conversation
Tony-Samuels
force-pushed
the
max_scale
branch
from
9d15ee0 to
1c430b5
Compare
Tony-Samuels
force-pushed
the
max_scale
branch
from
1c430b5 to
64213e8
Compare
src/decimal.rs
Outdated
| @@ -523,6 +525,7 @@ impl Decimal { | |||
| /// ``` | |||
| #[must_use] | |||
| pub const fn from_parts(lo: u32, mid: u32, hi: u32, negative: bool, scale: u32) -> Decimal { | |||
| assert!(scale <= Self::MAX_SCALE, "Scale would exceed maximum support scale"); | |||
There was a problem hiding this comment.
I guess looking back, it would've been nice to new type scale so that it could be validated there. I think if v2 ever materializes it is likely just a breaking clean up of the APIs.
On one hand, I'm with you that "undefined" behavior is breaking anyway - the fact that it was using a remainder I think emphasized the undefined nature of it.
On the other hand, while it was likely incorrect behavior before it didn't panic.
It's extremely difficult to know if downstream systems use this feature incorrectly, however assuming they did and attempting to put myself in their shoes - would I prefer an update to start panicking or continue behaving incorrectly?
That said: I think given the nature of this function (i.e. from raw parts) it is definitely an indication of advanced usage that I think should be fairly rare to utilize so it is probably ok to start asserting. My only suggestion would be a slight tweak:
| assert!(scale <= Self::MAX_SCALE, "Scale would exceed maximum support scale"); | |
| assert!(scale <= Self::MAX_SCALE, "Scale exceeds maximum supported scale"); |
src/fuzz.rs
Outdated
| @@ -8,7 +8,7 @@ impl Arbitrary<'_> for crate::Decimal { | |||
| let mid = u32::arbitrary(u)?; | |||
| let hi = u32::arbitrary(u)?; | |||
| let negative = bool::arbitrary(u)?; | |||
| let scale = u32::arbitrary(u)?; | |||
| let scale = u32::arbitrary(u)? % Self::MAX_SCALE + 1; | |||
There was a problem hiding this comment.
Should this be?
| let scale = u32::arbitrary(u)? % Self::MAX_SCALE + 1; | |
| let scale = u32::arbitrary(u)? % (Self::MAX_SCALE + 1); |
The only reason I suggest this is so that it includes fuzzing a scale of 0.
src/decimal.rs
Outdated
| } | ||
| }; | ||
|
|
||
| assert!( |
There was a problem hiding this comment.
Since this is a crate only function I'd perhaps remove this assert. Only because it can sometimes be useful to generate invalid scales etc.
Resolves #684.
Changes are as follows:
Decimal::MAX_SCALEinstead of a raw numberDecimal::MAX_SCALE, mostly just for consistency