-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
LibJS: Implement the "Change Array by copy" stage 3 proposal #14270
Conversation
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.
Nice work, couldn't really find anything but tiny nitpicks. And one commit order problem
Also while you're at it you could rename groupBy
to group
which was another new thing in the last meeting
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 good from having a quick glance over. I noticed that the tests don't check the contents of the original array are unmodified.
Userland/Libraries/LibJS/Tests/builtins/Array/Array.prototype.toReversed.js
Show resolved
Hide resolved
Userland/Libraries/LibJS/Tests/builtins/Array/Array.prototype.toSorted.js
Show resolved
Hide resolved
Userland/Libraries/LibJS/Tests/builtins/Array/Array.prototype.toSorted.js
Show resolved
Hide resolved
Userland/Libraries/LibJS/Tests/builtins/Array/Array.prototype.toSpliced.js
Show resolved
Hide resolved
Userland/Libraries/LibJS/Tests/builtins/Array/Array.prototype.toSpliced.js
Show resolved
Hide resolved
Userland/Libraries/LibJS/Tests/builtins/Array/Array.prototype.toSpliced.js
Show resolved
Hide resolved
Userland/Libraries/LibJS/Tests/builtins/Array/Array.prototype.toSpliced.js
Show resolved
Hide resolved
I just noticed while watching the video that this is missing tests for serenity/Userland/Libraries/LibJS/Tests/builtins/Array/Array.prototype.groupBy.js Lines 73 to 81 in c10d48b
|
Same for serenity/Userland/Libraries/LibJS/Tests/builtins/Array/Array.prototype.groupBy.js Lines 12 to 24 in c10d48b
|
d2b316e
to
aedf8cd
Compare
Thanks for the review both! I think I have all suggestions implemented 👍 |
This is covered by GlobalObject& just fine.
Also use it in array_merge_sort() instead of inlining the algorithm.
aedf8cd
to
7b5f94a
Compare
Part 1:
Array.prototype
functions