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

Implement i128/u128 to JsBigInt conversions #3129

Merged
merged 6 commits into from
Jul 17, 2023
Merged

Implement i128/u128 to JsBigInt conversions #3129

merged 6 commits into from
Jul 17, 2023

Conversation

AlvinKuruvilla
Copy link
Contributor

@AlvinKuruvilla AlvinKuruvilla commented Jul 10, 2023

This commit implements trait conversions to JsValue for i128/u128. This addresses #1970

This Pull Request fixes/closes #{1970}.

It changes the following:

  • Adds trait conversions for the missing i128 and u128 rust types to JsValue

AlvinKuruvilla and others added 2 commits July 10, 2023 01:19
This commit implements trait conversions to JsValue for i128/u128.
This addresses #1970
@Razican
Copy link
Member

Razican commented Jul 12, 2023

Hmmm I'm not sure if we want to do this. There is no native type in JavaScript equivalent to i/u128. And using a BigInt is not the same thing, since it's stored in the heap and performance is different.

I could see a TryFrom implementation that tries to convert it to an i32, though. Or to an f64 if there is no precision loss.

But in general, I wouldn't do this conversion, different users might have different thoughts on how should this work.

@AlvinKuruvilla
Copy link
Contributor Author

AlvinKuruvilla commented Jul 13, 2023

@Razican thanks for getting back to me.
Yeah, that makes sense, I saw this issue open and without activity, so I thought it might be a good way to get my feet wet. Is there no longer any interest in doing this, though?

@jedel1043
Copy link
Member

Hmmm I'm not sure if we want to do this. There is no native type in JavaScript equivalent to i/u128. And using a BigInt is not the same thing, since it's stored in the heap and performance is different.

Note that this PR implements From for JsBigInt and Numeric, not JsValue, and both traits are also implemented for i/u64.

@jedel1043
Copy link
Member

I'd support merging this change. Converting to Numeric is a lot more explicit about allocations than directly converting to JsValue, and if we want to make it a lot more explicit we could specify on the Numeric docs that all conversions from integer types allocate.

boa_engine/src/bigint.rs Outdated Show resolved Hide resolved
@jedel1043 jedel1043 changed the title Implement i128/u128 to JsValue conversions Implement i128/u128 to JsBigInt conversions Jul 16, 2023
@codecov
Copy link

codecov bot commented Jul 16, 2023

Codecov Report

Merging #3129 (57de737) into main (0cc4322) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #3129      +/-   ##
==========================================
- Coverage   50.65%   50.61%   -0.04%     
==========================================
  Files         444      444              
  Lines       42661    42696      +35     
==========================================
+ Hits        21608    21612       +4     
- Misses      21053    21084      +31     
Impacted Files Coverage Δ
boa_engine/src/bigint.rs 68.14% <0.00%> (-2.09%) ⬇️

... and 5 files with indirect coverage changes

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.

Looks good! just some small nitpicks :)

boa_engine/src/bigint.rs Outdated Show resolved Hide resolved
boa_engine/src/bigint.rs Outdated Show resolved Hide resolved
@HalidOdat HalidOdat added enhancement New feature or request API labels Jul 17, 2023
@HalidOdat HalidOdat added this to the v0.18.0 milestone Jul 17, 2023
@jedel1043 jedel1043 enabled auto-merge July 17, 2023 18:29
@jedel1043 jedel1043 added this pull request to the merge queue Jul 17, 2023
Merged via the queue into boa-dev:main with commit de192d3 Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants