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

Remove and cleanup functions in std::vec #9062

Closed
wants to merge 5 commits into from
Closed

Remove and cleanup functions in std::vec #9062

wants to merge 5 commits into from

Conversation

bluss
Copy link
Member

@bluss bluss commented Sep 9, 2013

Visit the free functions of std::vec and reimplement or remove some. Most prominently, remove each_permutation and replace with two iterators, ElementSwaps and Permutations.

Replace unzip, unzip_slice with an updated unzip that works with an iterator argument.

Replace each_permutation with a Permutation iterator. The new permutation iterator is more efficient since it uses an algorithm that produces permutations in an order where each is only one element swap apart, including swapping back to the original state with one swap at the end.

Unify the seldomly used functions build, build_sized, build_sized_opt into just one function build.

Remove equal_sizes

@bluss
Copy link
Member Author

bluss commented Sep 9, 2013

forgot to put the test in the correct spot in the file. Fixing that..

@alexcrichton
Copy link
Member

This... is awesome.

I'd be willing to r+ as-is, but I wanted to ask one question first:

  1. Perhaps vec::permutations would also be a nice way to expose this method? I'd be a little in favor of replacing Permutations::new to avoid confusion if that were the case. Alternatively we could even have v.permutations() where it's a method on vectors as well. What do you think?

I was also thinking we could make vec::unzip an iterator method instead of tying it to vectors, but that doesn't quite make sense to me, so nevermind.

@bluss
Copy link
Member Author

bluss commented Sep 9, 2013

unzip as a generic iterator would require returning two iterators that share state right, so that requires either Rc or Gc.

using a function or method instead of a ::new() function sounds good. The main issue with this iterator is the style of returning the same slice each iteration, it could be confusing (for example, the fix to the testcase below.. if it doesn't copy the vector with .to_owned() the hashset contains lots instances of just the same slice.

@bluss
Copy link
Member Author

bluss commented Sep 9, 2013

With these revelations, the new permutation iterator isn't much better than the old. An internal iteration function could encapsulate the efficient version however.

@alexcrichton
Copy link
Member

For what it's worth, this still seems better to me because it doesn't have any recursion and you can weave it with other iterators. It may not be able to drop Clone or the extra allocations just yet, but perhaps that can come soon.

@bluss
Copy link
Member Author

bluss commented Sep 9, 2013

It's not what it's supposed to be, but I think it's ready, now in the form of .permutations_iter(). I took the liberty of updating the main module documentation text for std::vec, I think it's better now.

blake2-ppc added 5 commits September 10, 2013 05:39
Remove unzip_slice since it's redundant. Old unzip is equivalent to the
`|x| unzip(x.move_iter())`
Introduce ElementSwaps and Permutations. ElementSwaps is an iterator
that for a given sequence length yields the element swaps needed
to visit each possible permutation of the sequence in turn.

We use an algorithm that generates a sequence such that each permutation
is only one swap apart.

    let mut v = [1, 2, 3];
    for perm in v.permutations_iter() {
        // yields 1 2 3 | 1 3 2 | 3 1 2 | 3 2 1 | 2 3 1 | 2 1 3
    }

The `.permutations_iter()` yields clones of the input vector for each
permutation.

If a copyless traversal is needed, it can be constructed with
`ElementSwaps`:

    for (a, b) in ElementSwaps::new(3) {
        // yields (2, 1), (1, 0), (2, 1) ...
        v.swap(a, b);
        // ..
    }
Update for a lot of changes (not many free functions left), add examples
of the important methods `slice` and `push`, and write a short bit about
iteration.
The basic construct x.len() == y.len() is just as simple.

This function used to be a precondition (not sure about the
terminology), so it had to be a function. This is not relevant any more.
These functions have very few users since they are mostly replaced by
iterator-based constructions.

Convert a few remaining users in-tree, and reduce the number of
functions by basically renaming build_sized_opt to build, and removing
the other two. This for both the vec and the at_vec versions.
@bluss
Copy link
Member Author

bluss commented Sep 10, 2013

I squashed together the permutation_iter changes, I updated the PR description title and text. A small diff from before the squash: I changed the text in the std::vec module doc slightly and added an assert to the test for ElementSwaps.

bors added a commit that referenced this pull request Sep 10, 2013
Visit the free functions of std::vec and reimplement or remove some. Most prominently, remove `each_permutation` and replace with two iterators, ElementSwaps and Permutations.

Replace unzip, unzip_slice with an updated `unzip` that works with an iterator argument.

Replace each_permutation with a Permutation iterator. The new permutation iterator is more efficient since it uses an algorithm that produces permutations in an order where each is only one element swap apart, including swapping back to the original state with one swap at the end.

Unify the seldomly used functions `build`, `build_sized`, `build_sized_opt` into just one function `build`.

Remove `equal_sizes`
@bors bors closed this Sep 10, 2013
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.

3 participants