Skip to content

Conversation

@ivandevp
Copy link
Contributor

Hey!

This PR adds Array.prototype.to_reversed method.

I tried to add a fast path similar to Array.prototype.reverse method but I wasn't able to figure out how to clone the array in order to not mutate the original array order. At least by doing array.clone() didn't work (it mutated the original array as well) and then the converting it to slice and back to an array triggers some issues between &[Option<Value>] and &[Value] that I couldn't find an easy fix - if we should add the fast path anyway, ideas are welcomed on how to implement it 🙏

@ivandevp ivandevp marked this pull request as ready for review October 16, 2024 19:30
Copy link
Member

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! Few minor issues, but all in all this is perfectly good to go.

Adding a fast path is possible, but requires a tad bit more involved work that touches on the Array's backing "elements heap vectors". This isn't too-too bad but requires a bit of work. I'll be happy to help if you want to take a stab at it.

@ivandevp ivandevp requested a review from aapoalas October 20, 2024 18:07
Copy link
Member

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some errors leaked in but overall this is indeed exactly how the fast path should work. Nice work! <3

@ivandevp ivandevp requested a review from aapoalas October 23, 2024 13:15
@ivandevp ivandevp requested a review from aapoalas October 24, 2024 12:53
aapoalas
aapoalas previously approved these changes Oct 29, 2024
Copy link
Member

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and thank you for the work and for the patience! Great stuff, and the shallow clone helper function will be of great help in the future with other toCloned array prototype methods.

I left a few minor things that could be changed, the comment thing being most important. But otherwise this looks very much good to go! <3

@aapoalas aapoalas force-pushed the feat/array-prototype-to_reversed branch from 5833071 to ec4f916 Compare November 8, 2024 05:44
@aapoalas
Copy link
Member

aapoalas commented Nov 8, 2024

Looks all good to me! Thank you very much, and sorry for the delay in reviewing! <3

@aapoalas aapoalas merged commit 2b98f83 into trynova:main Nov 8, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants