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.

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

Merged
merged 6 commits into from
Jun 10, 2020

Conversation

Tropid
Copy link
Contributor

@Tropid Tropid commented Jun 9, 2020

This Pull Request fixes/closes #415.

It changes the following:

  • Adds a BigInt.asUintN() function
  • Adds a BigInt.asIntN() function
  • Adds a to_index() function to the interpreter (https://tc39.es/ecma262/#sec-toindex)
  • Adds a to_integer() function to the interpreter (https://tc39.es/ecma262/#sec-tointeger, this is probably related to ValueData::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)
  • Adds a to_number() to the interpreter (https://tc39.es/ecma262/#sec-tonumber, this is also similar to value_to_rust_number which behaves a bit different)
  • Also adds a few tests (mostly from test262) for to_index, to_integer, BigInt.asIntN, BigInt.asUintN

@codecov
Copy link

codecov bot commented Jun 9, 2020

Codecov Report

Merging #468 into master will increase coverage by 0.68%.
The diff coverage is 96.52%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
boa/src/exec/mod.rs 65.57% <81.81%> (+2.96%) ⬆️
boa/src/builtins/bigint/mod.rs 82.22% <100.00%> (+15.55%) ⬆️
boa/src/builtins/bigint/operations.rs 54.83% <100.00%> (+10.01%) ⬆️
boa/src/builtins/bigint/tests.rs 100.00% <100.00%> (ø)
boa/src/exec/tests.rs 100.00% <100.00%> (ø)
boa/src/exec/conditional/mod.rs 83.33% <0.00%> (-16.67%) ⬇️
boa/src/exec/field/mod.rs 73.33% <0.00%> (-8.49%) ⬇️
boa/src/builtins/value/conversions.rs 42.42% <0.00%> (-0.44%) ⬇️
boa/src/builtins/value/equality.rs 63.85% <0.00%> (-0.10%) ⬇️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a78934d...56045cc. Read the comment docs.

@HalidOdat HalidOdat added builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request labels Jun 10, 2020
@HalidOdat HalidOdat added this to the v0.9.0 milestone Jun 10, 2020
Copy link
Member

@HalidOdat HalidOdat left a 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 :)

boa/src/builtins/bigint/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/bigint/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/bigint/mod.rs Outdated Show resolved Hide resolved
boa/src/exec/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@Razican Razican left a 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.

boa/src/exec/mod.rs Show resolved Hide resolved
boa/src/exec/mod.rs Show resolved Hide resolved
boa/src/exec/mod.rs Show resolved Hide resolved
boa/src/builtins/bigint/operations.rs Show resolved Hide resolved
Copy link
Member

@Razican Razican left a 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 :)

@Tropid
Copy link
Contributor Author

Tropid commented Jun 10, 2020

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)?

@Razican
Copy link
Member

Razican commented Jun 10, 2020

Thanks for the review and feedback. This is pretty much my first pull request on GitHub and I'm really amazed by the process.

You did great! Thanks for the contribution :)

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)?

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).

Copy link
Member

@HalidOdat HalidOdat left a 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

@HalidOdat HalidOdat merged commit a987e76 into boa-dev:master Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement BigInt static methods
3 participants