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

[Merged by Bors] - Fix length/index in 32bit architectures #2196

Closed
wants to merge 1 commit into from

Conversation

HalidOdat
Copy link
Member

@HalidOdat HalidOdat commented Jul 21, 2022

This PR fixes the length/index types from usize to u64, because in JavaScript the length can be from 0 to 2^53 - 1 it cannot be represented in a usize. On 64-bit architectures this is fine because it is already a unsigned 64bit number and these changes essentially do nothing. But on 32bit architectures this was causing the length to be truncated giving an unexpected result.

fixes/closes #2182

It changes the following:

  • Make length a u64 to be able to represent all possible length values
  • Change JsValue::to_length() t return u64
  • Change JsValue::to_index() t return u64

@HalidOdat HalidOdat added the bug Something isn't working label Jul 21, 2022
@HalidOdat HalidOdat added this to the v0.16.0 milestone Jul 21, 2022
@github-actions
Copy link

Test262 conformance changes

VM implementation

Test result main count PR count difference
Total 91,077 91,077 0
Passed 59,814 59,814 0
Ignored 14,134 14,134 0
Failed 17,129 17,129 0
Panics 0 0 0
Conformance 65.67% 65.67% 0.00%

@codecov
Copy link

codecov bot commented Jul 21, 2022

Codecov Report

Merging #2196 (09ee089) into main (79ea834) will decrease coverage by 0.01%.
The diff coverage is 35.61%.

@@            Coverage Diff             @@
##             main    #2196      +/-   ##
==========================================
- Coverage   41.93%   41.92%   -0.02%     
==========================================
  Files         231      231              
  Lines       21498    21499       +1     
==========================================
- Hits         9015     9013       -2     
- Misses      12483    12486       +3     
Impacted Files Coverage Δ
boa_engine/src/builtins/array/array_iterator.rs 86.66% <ø> (ø)
boa_engine/src/builtins/dataview/mod.rs 8.08% <ø> (ø)
...src/builtins/typed_array/integer_indexed_object.rs 0.00% <0.00%> (ø)
boa_engine/src/builtins/typed_array/mod.rs 3.75% <0.00%> (ø)
...ine/src/object/internal_methods/integer_indexed.rs 0.00% <0.00%> (ø)
boa_engine/src/object/jsarray.rs 5.15% <0.00%> (ø)
boa_engine/src/object/jsarraybuffer.rs 0.00% <0.00%> (ø)
boa_engine/src/builtins/string/mod.rs 63.94% <40.00%> (ø)
boa_engine/src/builtins/array_buffer/mod.rs 9.77% <42.85%> (ø)
boa_engine/src/builtins/array/mod.rs 75.71% <58.33%> (ø)
... and 9 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 79ea834...09ee089. Read the comment docs.

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.

Looks good! Thanks!

@raskad
Copy link
Member

raskad commented Jul 21, 2022

bors r+

bors bot pushed a commit that referenced this pull request Jul 21, 2022
This PR fixes the length/index types from `usize` to `u64`, because in JavaScript the length can be from `0` to `2^53 - 1` it cannot be represented in a `usize`. On `64-bit` architectures this is fine because it is already a unsigned 64bit number and these changes essentially do nothing. But on 32bit architectures this was causing the length to be truncated giving an unexpected result. 

fixes/closes #2182 

It changes the following:
- Make length a `u64` to be able to represent all possible length values
- Change `JsValue::to_length()` t return `u64`
- Change `JsValue::to_index()` t return `u64`
@bors
Copy link

bors bot commented Jul 21, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Fix length/index in 32bit architectures [Merged by Bors] - Fix length/index in 32bit architectures Jul 21, 2022
@bors bors bot closed this Jul 21, 2022
@bors bors bot deleted the fix/32bit-length branch July 21, 2022 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic compile to wasm32-unknown-unknown target
3 participants