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

feat(array-prototype): add splice method #451

Merged
merged 6 commits into from
Nov 11, 2024

Conversation

ivandevp
Copy link
Contributor

Hi!

This PR aims to add Array.prototype.splice method. Few tests are still failing but they're because of using uninplemented constructors like Proxy I think. Also, I haven't added a fast path - not sure if Vec::splice could be used for this purpose - any recommendation or guidance is highly appreciated.

@aapoalas
Copy link
Collaborator

Hey, and thank you again! From a quick glance looks like you're getting into the flow here and fairly easily cranking these out :)

A fast path here would be... First thing would be the usual "Object is an Array and it is trivial and dense" (wait, should it be "simple and dense"? I think "trivial" was a combination of "simple and dense"? anyway), of course. But together with that we should check that the start and delete counts are Integers. Then we can probably make up a few different internal fast paths:

  1. No deletions; effectively insert at index.
  2. No insertions; effectively delete at index.
  3. No deletions, no insertions (this doesn't need checking the Array at all actually); just return.
  4. Equal amount of deletions and insertions.
  5. More insertions than deletions.
  6. More deletions than insertions.

The first is best done with first a reserve for the amount of insertions to ensure that the Array internally uses a e2powX ElementsArray with enough room, followed by a length change to the new length after insertions: After this an array.as_slice_mut() will give you an slice of Values of the correct length where the end is filled with Nones. Now you just do a copy_within to push the "tail" of the original Array to the new end of the Array, and finally get a subslice of the "inserted data" and do a copy_from_slice with the arguments. (You'll need a transmute to turn the arguments' &[Value] into &[Option<Value>] but that is officially safe because Option<Value> is effectively a separate niche of the Value enum.)

The second is first a copy_within on the array.as_slice_mut() to overwrite the deleted items with the "tail" of the Array, and then a length set.

The third is of course just a return.

The fourth is a copy_from_slice on the array.as_slice_mut() from the arguments at the deletion index.

The fifth is a reserve and a length change and a copy_within on the surviving elements of the tail (that shouldn't get overwritten by insertions) to move them to the new end, and finally a copy_from_slice at the insertion index.

The sixth one is a copy_within to overwrite the "deleted part of the tail" with the surviving part of the tail, then a copy_from_slice and finally a length change.

These different parts can probably be combined into a single whole where most steps are conditional on some thing or another, but it's probably more efficient to split the code up like this; take on branch at the beginning and do the most of the work without extra branching.

@aapoalas
Copy link
Collaborator

aapoalas commented Nov 8, 2024

I handled the merge conflict: Do you want to take a stab at the fast path(s?) @ivandevp or should I merge this as-is?

@ivandevp
Copy link
Contributor Author

Hey @aapoalas , apologies for the late reply. Can I just fix the merge conflict for now and add the fast path in a separate PR? I got busy with current job change that I think I'm not going to have time to do it soon.

@aapoalas
Copy link
Collaborator

Hey @aapoalas , apologies for the late reply. Can I just fix the merge conflict for now and add the fast path in a separate PR? I got busy with current job change that I think I'm not going to have time to do it soon.

Of course, and don't worry about it! I'm really happy and thankful that you've contributed so much already, and especially with OSS there is absolutely no requirements here :) I can take care of the merge conflict as well if that's easier for you <3

@ivandevp
Copy link
Contributor Author

Thanks @aapoalas, I just fixed the merge conflicts :)

@aapoalas aapoalas merged commit 88b39ac into trynova:main Nov 11, 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