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

Fix Permutation's next function to work with Filter #4382

Merged
merged 2 commits into from
Oct 4, 2013

Conversation

astrieanna
Copy link
Contributor

See #4381

Filter does not work properly with Permutation because the next function modifies its arguments.
Adding a call to copy fixes this by not modifying the second argument.

@StefanKarpinski
Copy link
Member

It would be really nice to not have to copy this every iteration – that's a total performance killer.

@astrieanna
Copy link
Contributor Author

I don't know how else to maintain the assumption that next(itr,s) will not modify s.
Or is that not valid assumption, and I should instead be changing filter?

I guess it also makes no sense to have modifying (next!) and non-modifying (next) versions, since the second argument in many cases is an Int, which can't really have a next! function anyway. (Unless it's ok for the ! to mean "I might or might not modify the arguments, since I might not be able to, but I will return the right answer")

@StefanKarpinski
Copy link
Member

No, it's a problem – I want the intuitive behavior, I just don't want to give up the performance. See the previous discussion here, and related pull request discussions: #4055 and #4136. I guess it was a bit optimistic to think that mutating this thing wouldn't come back to bite us. cc: @kmsquire

JeffBezanson added a commit that referenced this pull request Oct 4, 2013
Fix Permutation's `next` function to work with Filter
@JeffBezanson JeffBezanson merged commit a11fd02 into JuliaLang:master Oct 4, 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