Skip to content
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.

Sign up for GitHub

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

Jump to bottom

BigInt to u128 #296

Merged
merged 3 commits into from
Jan 29, 2024
Merged

BigInt to u128 #296

merged 3 commits into from
Jan 29, 2024

Conversation

oskin1
Copy link
Contributor

@oskin1 oskin1 commented Jan 8, 2024

No description provided.

rooooooooob
rooooooooob approved these changes Jan 29, 2024
View reviewed changes
Copy link
Contributor

@rooooooooob rooooooooob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How far do we want to take this? I guess it's LGTM but to what point do we want to support all the explicit conversions? I guess u128 is fine. Do we want i128? I suppose those are the only two that would make sense since beyond that the are no default rust primitives, and below that there's the Int type.

@rooooooooob rooooooooob merged commit 6e37d57 into dcSpark:develop Jan 29, 2024
1 check passed
rooooooooob added a commit that referenced this pull request Jan 29, 2024
@rooooooooob
Fix for #302 / #296 conflict
7156ced 
I merged #296 thinking I would rebase #302 but before I could it was
merged.

This just renames BigInt -> BigInteger in the new BigInt tests
introduced in #296 which was done before the #302 renaming.
@rooooooooob rooooooooob mentioned this pull request Jan 29, 2024
Merged
rooooooooob added a commit that referenced this pull request Jan 30, 2024
@rooooooooob
Fix for #302 / #296 conflict (#303)
0940f0a 
I merged #296 thinking I would rebase #302 but before I could it was
merged.

This just renames BigInt -> BigInteger in the new BigInt tests
introduced in #296 which was done before the #302 renaming.
@SebastienGllmt
Copy link
Contributor

Although this PR was already merged, probably this wasn't the right way to implement this. Rust comes with From and TryFrom traits for cases like this, and the only reason we had the as_u128 pattern was because WASM needs explicit functions (which is no longer a concern with our Rust / WASM split)

This is not a big deal, but especially in cases like this where we only modified the Rust code and not the WASM code, we should probably do the more Rust-idiomatic approach

I created an issue for this: #304

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rooooooooob rooooooooob rooooooooob approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@oskin1 @SebastienGllmt @rooooooooob