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

Implement IntoIter for Matrix<N, R, C, S> where S: IntoIter, etc. #747

Closed
tpdickso opened this issue Jul 10, 2020 · 2 comments · Fixed by #1315
Closed

Implement IntoIter for Matrix<N, R, C, S> where S: IntoIter, etc. #747

tpdickso opened this issue Jul 10, 2020 · 2 comments · Fixed by #1315

Comments

@tpdickso
Copy link
Collaborator

I can whip up a PR for this if it's considered a desired feature.

This would make it easier to write iterators that flatten matrices, where some or all of these matrices may be temporary objects that need to be moved into the iterator so as not to be referencing a temporary. For example:

// How it can be written currently
let vertex_data = as.iter().zip(bs.iter()).flat_map(|(a, b)|
    a.cross(&b).data.into_iter()
).collect::<Vec<f32>>();

// How it could be written afterward
let vertex_data = as.iter().zip(bs.iter()).flat_map(|(a, b)|
    a.cross(&b).into_iter()
).collect::<Vec<f32>>();

I think this is a useful change, because having to dig into the matrix storage requires a deeper knowledge of the implementation details of Matrix, and is much less discoverable since there is no implementation of IntoIter for Matrix and solving it requires you to make the leap to the (hidden-by-default) data member to know that it does have an IntoIter implementation (which you can currently only find by going sideways through DefaultAllocator to ArrayStorage's Deref impl to GenericArray, manually googling it since the docs don't hyperlink types from external crates.)

Additionally, the current autoref to the IntoIter impl for &'a Matrix is unintuitive. If a user knows they have an owned MatrixMN, VectorN or PointN, it's intuitive that they should be able to convert it to an owned iterator, and it's confusing for it to borrow instead.

However, it would be a breaking change. The rust language itself is phasing in a similar breaking change where [T; N].into_iter() takes ownership of the array instead of auto-ref'ing to a slice iterator, so there is precedent for this.

@jan-grimo
Copy link

I got surprised by this behavior recently. I would favor making this change, and would also be willing to make the PR (though I'd need some pointers on how to proceed).

@Ralith
Copy link
Collaborator

Ralith commented May 4, 2023

I just ran into this myself. It'd be similarly helpful to have IntoIter for views, yielding &N.

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 a pull request may close this issue.

3 participants