Implement TypedArray.prototype.toReversed
#1366
Conversation
facebook-github-bot
added
the
CLA Signed
|
@dannysu has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
robik
force-pushed
the
typed-array-es14-to-reversed
branch
from
4e9bb1b to
76da167
Compare
|
@robik has updated the pull request. You must reimport the pull request before landing. |
lib/VM/JSLib/TypedArray.cpp
Outdated
|
|
||
| // 5. Let k be 0. | ||
| double k = 0; | ||
| MutableHandle<> kHandle{runtime}; |
There was a problem hiding this comment.
The unused variable is still here.
There was a problem hiding this comment.
Oh damn, sorry, Must have re-added it on rebase I think
|
@dannysu has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
robik
force-pushed
the
typed-array-es14-to-reversed
branch
from
76da167 to
a766b77
Compare
|
@robik has updated the pull request. You must reimport the pull request before landing. |
lib/VM/JSLib/TypedArray.cpp
Outdated
| JSArrayBuffer::size_type from = len - k - 1; | ||
|
|
||
| // 6d. Perform ! Set(A, Pk, fromValue, true). | ||
| JSArrayBuffer::copyDataBlockBytes(runtime, aBuffer, k * byteWidth, srcBuffer, from * byteWidth, byteWidth); |
There was a problem hiding this comment.
Could you get the underlying data pointer at above lines and use memcpy here directly? And you can further change k and from to be index in the underlying data array, so you don't need to do multiplications here.
There was a problem hiding this comment.
WDYT about creating a static utility function for copying elements in JSTypedArray (where byte width is known) rather than copying bytes?
There was a problem hiding this comment.
Then you have to specialize it for every valid width. Probably not necessary at the moment. It can be done later if we do need to dig more performance. For now, memcpy on the underlying data_ array is good enough.
|
For the other PRs, we should eliminate |
|
Sure thing! I just wanted to make sure that the current approach is correct first. If it is, I can apply it to other methods :) |
|
@robik has updated the pull request. You must reimport the pull request before landing. |
robik
force-pushed
the
typed-array-es14-to-reversed
branch
from
cca70ae to
165fb0b
Compare
|
@robik has updated the pull request. You must reimport the pull request before landing. |
Summary
This PR implements ES2023
TypedArray.prototype.toReversedmethod. This is a follow-up of PR #1286 which added with method on standard Array prototype.See also:
TypedArray.prototype.with: ImplementTypedArray.prototype.with#1365TypedArray.prototype.toSorted: ImplementTypedArray.prototype.toSorted#1367I am creating separate PRs for each method in case there are any suggestions or change requests from your side, so that PRs are more independent.
Test Plan
Code is annotated with algorithm steps from EcmaScript specification for easier verification and maintenance.
I also added tests to verify that methods work as intended. There might be some more edge cases that might be covered based on your experience.