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

RFC: prevent mapslices from mutating the original array #17266

Merged
merged 1 commit into from
Jul 5, 2016

Conversation

timholy
Copy link
Member

@timholy timholy commented Jul 4, 2016

This is a broader fix for #17153, making it so that mapslices never modifies the input array. It allocates temporary storage and copies each slice into it before calling the user-function.

@simonster commented (#17153 (comment)) that this would still be breaking compared to the old behavior (which created a new array with A[idx...] on every iteration), but except for "pathalogical functions" (e.g., those whose behavior might depend on the array's pointer address) I confess I fail to see how this is the case.

@timholy timholy changed the title Prevent mapslices from mutating the original array RFC: prevent mapslices from mutating the original array Jul 4, 2016
@timholy
Copy link
Member Author

timholy commented Jul 4, 2016

I should have marked this "RFC" since the proper behavior is of course debatable.

@simonster
Copy link
Member

The case I was imagining was pushing slices into an Array and expecting to have n different slices instead of the last slice n times when mapslices returns. I'm not aware of any code that does this, but it's possible.

@simonster
Copy link
Member

If this gets merged, we should also change the description of the breaking change to mapslices in NEWS.md, and maybe revert the code changes (but not the tests) from #17154.

@timholy
Copy link
Member Author

timholy commented Jul 4, 2016

Thanks for explaining. I agree this would break such functions, but (as you seem to feel) I don't think we need to bend over backwards to support such uses.

@tkelman
Copy link
Contributor

tkelman commented Jul 4, 2016

If this gets merged, we should also change the description of the breaking change to mapslices in NEWS.md, and maybe revert the code changes (but not the tests) from #17154.

should probably do that here before merging

@tkelman tkelman closed this Jul 4, 2016
@tkelman tkelman reopened this Jul 4, 2016
@timholy timholy force-pushed the teh/mapslices_copy branch from 35d7564 to d9ce339 Compare July 4, 2016 20:10
@timholy
Copy link
Member Author

timholy commented Jul 5, 2016

Looks like there are no real objections to this change in strategy.

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