Fixing permutation of small lists, such that [], [x] -> [[]], [[x]] #13783
Conversation
| @@ -388,6 +398,7 @@ impl<T: Clone> Iterator<~[T]> for Permutations<T> { | |||
| fn next(&mut self) -> Option<~[T]> { | |||
| match self.swaps.next() { | |||
| None => None, | |||
| Some((0,0)) => Some(self.v.clone()), | |||
There was a problem hiding this comment.
Did you find it necessary to have this case? I think that swap handles the case where the two indices are the same.
There was a problem hiding this comment.
@alexcrichton: I had two reasons:
- For an empty list, there is no item 0, and that would be bad.
- (Not as important) For a non-empty list, AFAICT there is no code in either
slice::swaporptr::swapstopping it from actually performing the swap, i.e. copying item 0 to a temporary value, moving item 0 to item 0, and then copying the temp value back in. That seemed unnecessary...
There aren't necessarily N! unique permutations, but the current implementation does not worry about uniqueness. |
| // However, we don't know how many are left, so the lower bound is 0. | ||
| let n = range(2, self.v.len() + 1).product(); | ||
| (0, Some(n)) | ||
| } |
There was a problem hiding this comment.
Could the size hint possibly be defined on ElementSwaps instead, and have Permutations::size_hint just call through to that?
There was a problem hiding this comment.
@kballard: Sure, it could; is there a reason to do so?
There was a problem hiding this comment.
ElementSwaps is a public type. If someone wants to use it independently of Permutations it would be nice to have a size_hint().
There was a problem hiding this comment.
OK, I'll work on that.
There was a problem hiding this comment.
@kballard: Question: when I commit that, should I rebase that again (wiping out the old commit), or just leave it on top of the given commit?
There was a problem hiding this comment.
Amend the commit and force-push. There's no benefit to keeping it around as a separate commit.
There was a problem hiding this comment.
Great, thanks, that's now done!
|
@wackywendell Ah, you're very much right. That's another difference between the current implementation and my proposed one; since it works with lexicographic permutations, it returns unique ones only. |
|
Sorry about that, hit the wrong button... |
| else { | ||
| return None; | ||
| } | ||
| } |
There was a problem hiding this comment.
It seems to me this block should be removed and instead put later in the match max block (line 373 of this current patch).
Something like:
None => if self.emit_reset {
self.emit_reset = false;
if self.sdir.len() > 1 { Some((0, 1)) } else { Some((0, 0)) }
} else {
None
}There was a problem hiding this comment.
Could you explain why? I'm not sure I see why it matters much either way.
There was a problem hiding this comment.
@wackywendell Not only is it simpler code, but it avoids the unnecessary branch on calls to next() for lists of size larger than 1.
There was a problem hiding this comment.
Done! You're right, it is simpler.
|
r=me once my last few comments are addressed. I think it makes sense to make |
|
@kballard I just added both - a counter in |
I filed bugs #13734 and #13759 recently, and then realized I could probably fix them myself. This does exactly that, with a couple additional modifications and additions to the test-suite to pick up on that. I've never done this before, so please feel free to tell me all the things I'm doing wrong or could be doing better.
Add parentheses for binding mode hints when they attach to an Or-pattern
I filed bugs #13734 and #13759 recently, and then realized I could probably fix them myself. This does exactly that, with a couple additional modifications and additions to the test-suite to pick up on that.
I've never done this before, so please feel free to tell me all the things I'm doing wrong or could be doing better.