feat(primitivies): impl a handful of traits for fixed types #7565
Conversation
saiintbrisson
force-pushed
the
feat/primitivies/impl-sum-for-fixed-types
branch
3 times, most recently
from
9ea4117 to
85d920e
Compare
saiintbrisson
force-pushed
the
feat/primitivies/impl-sum-for-fixed-types
branch
from
85d920e to
3944b6c
Compare
|
Hi, @bkchr, sorry for the ping. Can I get your attention here? |
| @@ -175,7 +175,7 @@ pub trait FixedPointNumber: | |||
| d: D, | |||
| ) -> Option<Self> { | |||
| if d == D::zero() { | |||
| return None | |||
| return None; | |||
There was a problem hiding this comment.
Probably your IDE doing this; but you can improve review speed by omitting unrelated changes.
| @@ -513,6 +513,11 @@ macro_rules! implement_fixed { | |||
| self.0 as f64 / <Self as FixedPointNumber>::DIV as f64 | |||
| } | |||
|
|
|||
| /// Convert into a `u32` value. | |||
There was a problem hiding this comment.
Can you add a comment on the rounding mode please? Seems to be Rounding::Down.
| @@ -926,6 +969,18 @@ macro_rules! implement_fixed { | |||
| } | |||
| } | |||
|
|
|||
| impl ::core::iter::Sum for $name { | |||
| fn sum<I: Iterator<Item = Self>>(iter: I) -> Self { | |||
| iter.fold(Self::zero(), |a, b| a + b) | |||
| assert_eq!(fixed.0, (u32::MAX as $inner_type) * $name::DIV); | ||
|
|
||
| let val = fixed.to_u32(); | ||
| assert_eq!(val, u32::MAX); |
There was a problem hiding this comment.
Can you add some more cases here please? Maybe also a random generated value in a loop.
We have some fuzz tests in sp-arithmetic-fuzzer, probably overkill to add it there, just saying.
| @@ -2204,6 +2318,50 @@ macro_rules! implement_fixed { | |||
| }; | |||
| } | |||
|
|
|||
| macro_rules! impl_signed_trait { | |||
| return None | ||
| } | ||
| let (result, remainder) = Double128::product_of(a, b).div(c); | ||
| let Some((result, remainder)) = multiply_by_rational(a, b, c) else { |
|
@saiintbrisson can you answer the questions? |
|
I have opened #8472 which introduces multiple checked functions and new fuzzing tests. To audit, there is no conflict between the introduced functions, so either of both could merge first. As I mentioned in the other PR, the whole crate is audited now, so I can start doing diff sec review of introduced changes to the crate once mine is merged and there are not many PR that I can't keep up with. cc/ @ggwpez |
|
Hi, I've been away for a while. I'll see what I can do in the coming weeks. @Gioyik let's have yours merged first! And then I'll adapt changes here. Sorry for the absence. |
This change implements a handful of traits for fixed types:
sumhelper for iterators,Rem/CheckedRem,Signed/UnsignedandNum,MultiplyArgto be used withPerThing,to_u32to matchto_float.I'd also love to impl forward reference implementations for binops traits (
Add<&Self> for Self,Add<Self> for &Self, etc), but this breaks the existing.into()calls as it turns the conversion ambiguous. Not sure if that's a price you are willing to pay, but would be very much appreciated.