Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support BigInt for i64 and u64 #24
Support BigInt for i64 and u64 #24
Changes from 3 commits
37b3805
8baa1fa
ab0c892
4087cd8
69af2da
ea7ddeb
90dcf81
05a8010
6b6e901
c43908a
642a472
053845d
bfbfa3e
23d1dd7
067e8d0
cec3fb5
44153b7
d171675
f416074
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I'd suggest to use
.dyn_into()
here instead to combine theis_*
check and.into()
cast (which does its own check internally) into 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.
This one is still using strings, but shouldn't anymore.
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.
oops
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.
(here and around) please make sure to run
cargo fmt
in the end, or, better yet, you might want to configure format-on-save in your editor.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.
Same here about missing
assert_eq!
or something.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.
Can you also please add a few tests for conversion from a
BigInt
that doesn't actually fit into 64 bits? (you can create one from a string, it's fine to do so in tests)It should throw an error, but I suspect right now it's not handled correctly.
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.
I added a js file to do validation on the BigInt's. I think it is the easiest and lowest overhead way of handling the bounds check. It replaces the
to_i64
andto_u64
bindings used previouslyThere 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.
Unfortunately, the JS integration doesn't work in all wasm-bindgen modes, see Caveats. While I agree it would be lower overhead, I'd rather keep this crate working for all output modes, as otherwise it's a pretty significant breaking change.
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.
As mentioned in the initial comment where I proposed the bindings idea, you can just compare the resulting
u64
with the originalBigInt
instance. It will require one more roundtrip to JS but shouldn't be that bad and will be working in all modes.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.
I missed the lack of nodejs support while reading that page of the docs. I appreciate all of the help, I am pretty new to wasm-bindgen.