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

Add Enum.slide/3 #11349

Merged
merged 5 commits into from Oct 31, 2021
Merged

Add Enum.slide/3 #11349

merged 5 commits into from Oct 31, 2021

Conversation

s3cur3
Copy link
Contributor

@s3cur3 s3cur3 commented Oct 27, 2021

[Below is my intro/pitch from the previous discussion on the elixir-lang-core mailing list.]

Rotate pulls a slice of an enumerable out and replaces that slice somewhere else.

The classic use case is this: Suppose you have a list of to-do items, which the user has ordered by priority:

  1. Apply to college
  2. Brush the dog
  3. Change the car's oil
  4. Deliver flowers
  5. Exchange gifts

A rotate occurs when the user selects some number of elements and drags them to a new place in the list. Let's say they selected items 3 & 4 from the preceding and dragged them above item 2. When they release the mouse, the new order should be:

  1. Apply to college
  2. Change the car's oil
  3. Deliver flowers
  4. Brush the dog
  5. Exchange gifts

Doing this without the named algorithm requires 3 calls to Enum.slice/3 (one at the insertion point, one at the start of the selected range, and one at the end of the selected range). It's easy to get the index math wrong, and it's even harder for readers of your code to grasp what's going on... and it's also substantially slower than the implementation here.

A number of other languages have a rotate algorithm, though it's still somewhat uncommon. I found Dave Abrahams' comments valuable when this was discussed for inclusion in Swift.

Previous discussion from the elixir-lang-core mailing list:
https://groups.google.com/g/elixir-lang-core/c/LYmkUopaWN4/m/SwBzRt2zBQAJ
@michalmuskala
Copy link
Member

This does look useful, but looking at the description and comparing the implementation with the C++ and Rust functions, this seems to me like it performs a completely different operation.

In particular the C++ std::rotate(first, n_first, last) is defined as " Specifically, std::rotate swaps the elements in the range [first, last) in such a way that the element n_first becomes the first element of the new range and n_first - 1 becomes the last element..". The behaviour in Rust is similar (though you only give the n_first argument, first and last is implied by the slice range.

I find it relatively hard to understand how a C++ call to the rotate function could be translated to this Elixir implementation.

@s3cur3
Copy link
Contributor Author

s3cur3 commented Oct 29, 2021

Right... the C++ equivalent I've heard called "slide," and it's just a wrapper around rotate to do the correct ordering of your range versus the insertion point (such that the 3 indices passed to rotate are in strictly increasing order). There was some discussion on the mailing list about whether we should go with another name. I don't have a particularly strong preference there. 🤷‍♂️

@josevalim
Copy link
Member

Just a quick note before I can review the PR, but I think slide may be a better name indeed. rotate somehow gives me an idea of mutation, but slide feels like a transformation. Does this make sense? WDYT?

@s3cur3
Copy link
Contributor Author

s3cur3 commented Oct 29, 2021

Slide works for me! Pushing the change now...

@s3cur3 s3cur3 changed the title Add Enum.rotate/3 Add Enum.slide/3 Oct 29, 2021
iex> Enum.slide([0, 1, 2, 3, 4, 5, 6], 3..-1//1, 2)
[0, 1, 3, 4, 5, 6, 2]
iex> Enum.slide([0, 1, 2, 3, 4, 5, 6], -4..-2, 1)
[0, 3, 4, 5, 1, 2, 6]
Copy link
Member

Choose a reason for hiding this comment

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

I just realized something, by having the input list be numbers, someone can think that 1..3 is referring to the actual values and not the position. Shall we use [:a, :b, :c, ...] as input?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense!

lib/elixir/lib/enum.ex Outdated Show resolved Hide resolved
lib/elixir/lib/enum.ex Outdated Show resolved Hide resolved
end

{index + 1, pre, post}
end)
Copy link
Member

Choose a reason for hiding this comment

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

Beautiful! 💯


# Like slide_any/4 above, this optimized implementation of slide for lists depends
# on the indices being sorted such that we're moving middle..last to be in front of start.
defp find_start([h | t], start, middle, last)
Copy link
Member

Choose a reason for hiding this comment

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

Let's call this slide_list_start and then slide_list_middle and slide_list_last?

@josevalim
Copy link
Member

@s3cur3 thank you for the PR! ❤️ the code looks great, I have dropped some minor comments on the docs and on the function naming, everything else is ready to go!

@s3cur3
Copy link
Contributor Author

s3cur3 commented Oct 31, 2021

Really appreciate the code review, @josevalim. ☺️ I've applied all your changes. 👍

@josevalim
Copy link
Member

If you are looking for a challenge, you can implement Stream.slide :) I believe the implementation would fit Stream.Reducers, so it should be a matter of converting the Enumerable implementation into the Stream.Reducers format!

@s3cur3
Copy link
Contributor Author

s3cur3 commented Oct 31, 2021

Sweet, I'll look into it! 👍

@josevalim josevalim merged commit c1d2bc7 into elixir-lang:master Oct 31, 2021
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants