-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
Add BigInt.asIntN() and BigInt.asUintN() functions #468
Conversation
Codecov Report
@@ Coverage Diff @@
## master #468 +/- ##
==========================================
+ Coverage 66.39% 67.07% +0.68%
==========================================
Files 146 147 +1
Lines 9350 9544 +194
==========================================
+ Hits 6208 6402 +194
Misses 3142 3142
Continue to review full report at Codecov.
|
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 looks awesome! check my comments to see how we can improve it :)
Co-authored-by: HalidOdat <halidodat@gmail.com>
Co-authored-by: HalidOdat <halidodat@gmail.com>
Co-authored-by: HalidOdat <halidodat@gmail.com>
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'm only missing a bit of documentation. Benchmarks failing was a small bug introduced in the benchmark compare action that should be fixed in the next rebuild.
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 is good to go for me :)
Thanks for the review and feedback. This is pretty much my first pull request on GitHub and I'm really amazed by the process. Should I rebase the commits, so there are not as many small commits like "Remove extra comment in doc comment" and "Remove another extra comment in doc comment" (I did not see the "Add suggestion to batch" button at first)? |
You did great! Thanks for the contribution :)
There is no need, we squash and merge all commits together when we merge :) The only good thing about rebasing is that benchmarks will start showing here as comments. That's something we might change in the future, though (the fact that it requires for the PR to be rebased). |
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 Looks perfect! Thanks for the contribution @Tropid
This Pull Request fixes/closes #415.
It changes the following:
BigInt.asUintN()
functionBigInt.asIntN()
functionto_index()
function to the interpreter (https://tc39.es/ecma262/#sec-toindex)to_integer()
function to the interpreter (https://tc39.es/ecma262/#sec-tointeger, this is probably related toValueData::to_integer
which seems to behave different than the function described in the standard, it also returns a 32 bit integer instead of a 64 bit integer)to_number()
to the interpreter (https://tc39.es/ecma262/#sec-tonumber, this is also similar tovalue_to_rust_number
which behaves a bit different)to_index
,to_integer
,BigInt.asIntN
,BigInt.asUintN