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

Make easier to add shuffle to other types #1132

Merged
merged 10 commits into from
Nov 6, 2023
Merged

Make easier to add shuffle to other types #1132

merged 10 commits into from
Nov 6, 2023

Conversation

leighmcculloch
Copy link
Member

@leighmcculloch leighmcculloch commented Nov 5, 2023

What

Change how shuffle is implemented and provided so that the functionality is provided by implementors of a trait.

Also, change how it works so that it mutates the existing value. But provide a to_shuffled function as well.

Why

There's a few reasons for the different changes that were difficult to break apart which is why I've submitted them all in this one PR.

Providing the shuffle functionality via a trait allows us to define shuffle behavior on a wider variety of types, other than only types that implement IntoVal<_, Vec<Val>>. For example, we'd be able to provide the functionality on [T; N] as well or some other types in the future.

Providing the shuffle functionality as a mutation of the type also provides some optimizations in cases where the developers wants to shuffle something without allocating a new copy of it on the stack or elsewhere. Even though the functionality is changed to mutate, it's still possible for the Vec case for us to include a to_shuffled function that provides a copy. In the end this communicate clearer.

I implemented ToShuffled for all Shuffle, but maybe that's a mistake. Blanket implementations often come back to bite us.

Note

This is a breaking change. There isn't really a good way to deprecate this and reuse the fn names. We're still in pre-release though, so I think that's okay.

@leighmcculloch leighmcculloch marked this pull request as ready for review November 5, 2023 22:26
@leighmcculloch leighmcculloch added this pull request to the merge queue Nov 6, 2023
Merged via the queue into main with commit 81f96a3 Nov 6, 2023
@leighmcculloch leighmcculloch deleted the rng-shuffle branch November 6, 2023 04:31
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