feat!: replace U128::from_integer with From impls and U128::from_field
#4944
Conversation
github-actions
bot
added
the
documentation
|
🚀 Deployed on https://663b84cbd7d2d92a129bb36a--noir-docs.netlify.app |
|
FYI @noir-lang/developerrelations on Noir doc changes. |
TomAFrench
force-pushed
the
tf/impl-from-on-u128
branch
from
aa4a6f6
to
4fb65ee
Compare
|
This is blocked by #4944 |
noir_stdlib/src/uint128.nr
Outdated
| impl From<u64> for U128 { | ||
| fn from(value: u64) -> U128 { | ||
| U128::from_u64s_le(value, 0) | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you add a from_u64 in addition to from_field? I think it'd make this easier to find for users
| U128 { lo, hi } | ||
| } | ||
|
|
||
| pub fn to_integer<T>(self) -> T { | ||
| crate::from_field(self.lo + self.hi * pow64) | ||
| crate::from_field(self.lo + self.hi * POW_64) |
There was a problem hiding this comment.
Do we have plans to remove from_field and as_field eventually? It seems they'd be better replaced by the From trait.
There was a problem hiding this comment.
I've kept from_field and not implemented From<Field> as this conversion can fail.
There was a problem hiding this comment.
I think we may want to have something along the lines of an AssertFrom trait which is essentially From but will throw a constraint error if the conversion isn't valid.
There was a problem hiding this comment.
Ah, I was just thinking of TryFrom
There was a problem hiding this comment.
How would TryFrom work here? I'm thinking of AssertFrom as we have a number of conversions where it's much easier to assert that the conversion is valid rather than checking whether it is valid or not.
There was a problem hiding this comment.
TryFrom would always be similar to AssertFrom, it just gives users the option to recover from an error if possible but would be less efficient than the AssertFrom in cases where users do just want to panic on failure.
|
Yep, just noticed I copy-pasted incorrectly there. I'll add |
| Conversion between unsigned integer types and U128 are done through the use of the `From` trait and `to_integer` method. There also exists a `from_field` method which accepts a `Field` type as input (which is asserted to fit within a `U128`). | ||
|
|
||
| ```rust | ||
| fn main() { | ||
| let x = U128::from_integer(23); | ||
| let x = U128::from(23); |
There was a problem hiding this comment.
This needs updating to use from_u64 as the From trait is broken
|
This performance regression on |
|
From the CI itself it looks like although there are more ACIR instructions the overall circuit size is still smaller for u128 at least. |
Description
Problem*
Resolves
Summary*
This PR replaces the overly wide generic function
U128::from_integerwithFromimpls foru1,u8,u32, andu64. I've also addedU128::from_fieldto maintain the ability to create aU128from fields.Additional Context
Opened #4943 based off of issues when making this PR.
Documentation*
Check one:
PR Checklist*
cargo fmton default settings.