-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
[exec Array] implement spread operator using iterator #811
[exec Array] implement spread operator using iterator #811
Conversation
Hey! thanks for this contribution. It seems it requires a re-base to fix one issue we were having in the test262 test suite :) |
Codecov Report
@@ Coverage Diff @@
## master #811 +/- ##
==========================================
- Coverage 59.37% 59.26% -0.12%
==========================================
Files 157 157
Lines 10036 10040 +4
==========================================
- Hits 5959 5950 -9
- Misses 4077 4090 +13
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good to me. Check if the mentioned changes make sense.
boa/src/syntax/ast/node/array/mod.rs
Outdated
//not sure what to do with this next_index mentioned in the spec | ||
//it is mentioned that it has to be returned from somewhere | ||
//https://tc39.es/ecma262/#sec-runtime-semantics-arrayaccumulation | ||
//let mut next_index = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is basically because we don't follow the spec with arrays. The thing is, arrays have a number of integer-indexed elements, and a nextIndex
. this nextIndex
will be the same as array.length
, and it's defined here: https://tc39.es/ecma262/#sec-array-initializer-runtime-semantics-evaluation
But, due to our array being a vector (which causes performance issues), we don't have this defined. This is partially the reason behind ##816. This means that if we for example create a huge empty array (which should not allocate), say new Array(50000)
, we will have huge performance issues.
This also means that if we do something like let a = [5,6,,,6,7,,8];
, we will have an undefined
value in the non-defined slots, but they should instead be empty slots, and here, nextIndex
would be 8 (same as length).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you suggest? Should we leave this comment and commented next_index, or modify the comment, or remove completly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would change the comment to a TODO
, explaing this is waiting for a proper internal Array representation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the newly committed rewording.
This Pull Request fixes #793
It changes the following:
Map
spreading, because Map does not implement iterator. That's why I opened Map.prototype[@@iterator]() method not implemented #810