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

Fix of instructions.rs comment, to_precision impl and rfc changes #1135

Merged
merged 8 commits into from
May 24, 2021

Conversation

NathanRoyer
Copy link
Contributor

@NathanRoyer NathanRoyer commented Feb 18, 2021

This Pull Request fixes/closes #1038.

It changes the following:

Copy link
Contributor

@RageKnify RageKnify left a comment

Choose a reason for hiding this comment

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

Code looks good, but I think it'd be nice to have 1 or 2 test cases for the change you did to round_to_precision to make sure no regressions are introduced in the future. Is it easy to do that @NathanRoyer?

@RageKnify
Copy link
Contributor

test262 numbers actually look bad

Test262 conformance changes:

Test result master count PR count difference
Total 78,497 78,497 0
Passed 24,986 24,984 -2
Ignored 15,587 15,587 0
Failed 37,924 37,926 +2
Panics 16 16 0
Conformance 31.83 31.83 -0.00%

@Razican
Copy link
Member

Razican commented Mar 10, 2021

This would need a rebase in order for the build to pass.

@RageKnify RageKnify added this to the v0.12.0 milestone Mar 29, 2021
@RageKnify RageKnify added bug Something isn't working builtins PRs and Issues related to builtins/intrinsics labels Mar 29, 2021
@RageKnify
Copy link
Contributor

@NathanRoyer this directory has the test262 tests for Number.prototype.toPrecision, can you take a look to try to understand what is now failing that wasn't before? You could also use the test runner if you prefer.

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 suggest changing a couple of things. The code looks good, but as @RageKnify mentions, you should see why there are 2 new tests failing.

Also, it needs a rebase, again.

boa/src/builtins/math/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/string/mod.rs Outdated Show resolved Hide resolved
@Razican Razican modified the milestones: v0.12.0, v0.13.0 May 22, 2021
@0x7D2B
Copy link
Contributor

0x7D2B commented May 23, 2021

Latest test262 results:

Test262 conformance changes:

Test result master count PR count difference
Total 78,873 78,873 0
Passed 26,498 26,502 +4
Ignored 15,604 15,604 0
Failed 36,771 36,767 -4
Panics 27 27 0
Conformance 33.60% 33.60% +0.01%

It doesn't post the comment but you can find the text in the Update Comment section of test262 results: https://github.com/boa-dev/boa/pull/1135/checks?check_run_id=2649202840

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.

Benchmarks look pretty good, and conformance too. It's good to merge from my side :) thank you!

@Razican Razican merged commit 88e888f into boa-dev:master May 24, 2021
@Razican Razican modified the milestones: v0.13.0, v0.12.0 May 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working builtins PRs and Issues related to builtins/intrinsics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Seemingly wrong comment for Rational in instruction enum
4 participants