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

Implement lossless TryFromJs for integers from f64 #3907

Merged
merged 2 commits into from
Jul 10, 2024
Merged

Implement lossless TryFromJs for integers from f64 #3907

merged 2 commits into from
Jul 10, 2024

Conversation

HalidOdat
Copy link
Member

Implement conversion from floating point values that can be converted to integer values losslessly.

The spec does not distinguish between integer numbers and floating point, so I don't think 4 should succeed and 4.0 fail, according to the spec they have the same type and are equal.

@HalidOdat HalidOdat added the enhancement New feature or request label Jul 9, 2024
@HalidOdat HalidOdat requested a review from a team July 9, 2024 17:25
@HalidOdat HalidOdat mentioned this pull request Jul 9, 2024
Open
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 9, 2024 •
edited
Loading

Test262 conformance changes

Test result main count PR count difference
Total 48,212 48,212 0
Passed 42,875 42,875 0
Ignored 1,413 1,413 0
Failed 3,924 3,924 0
Panics 0 0 0
Conformance 88.93% 88.93% 0.00%

nekevss
nekevss approved these changes Jul 10, 2024
View reviewed changes
Copy link
Member

@nekevss nekevss left a comment

Choose a reason for hiding this comment

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

I had one potential discussion point, but it's nothing that would block merging for me.

core/engine/src/value/conversions/try_from_js.rs Outdated Show resolved Hide resolved
@HalidOdat HalidOdat requested review from jedel1043 and a team July 10, 2024 18:03
jedel1043
jedel1043 approved these changes Jul 10, 2024
View reviewed changes
Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

LGTM!

@jedel1043 jedel1043 enabled auto-merge July 10, 2024 18:10
@jedel1043 jedel1043 added this pull request to the merge queue Jul 10, 2024
Merged via the queue into main with commit 5b1621a Jul 10, 2024
13 checks passed
@jedel1043 jedel1043 deleted the feature/lossless-try_from_js-floating-to-integer branch July 10, 2024 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nekevss nekevss nekevss approved these changes

@jedel1043 jedel1043 jedel1043 approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@HalidOdat @jedel1043 @nekevss