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

Add Clone to MatrixIter and MatrixIterMut #1251

Merged
merged 2 commits into from
Jul 8, 2023
Merged

Add Clone to MatrixIter and MatrixIterMut #1251

merged 2 commits into from
Jul 8, 2023

Conversation

Shatur
Copy link
Contributor

@Shatur Shatur commented Jun 27, 2023

Could be needed for crates like https://docs.rs/logaddexp/latest/logaddexp/.
Other iterators in nalgebra also have Clone impl.

@Andlon
Copy link
Sponsor Collaborator

Andlon commented Jun 27, 2023

I don't think it's sound for MatrixIterMut to implement Clone, since this could allow you to have two separate mutable iterators for the same mutable matrix entries, from which you could recover two mutable references to the same element in the matrix.

For MatrixIter I suppose it could be sound. But directly deriving might not be desirable since it will add a Clone bound for every generic parameter. In this case it's better to do a manual impl with only the bounds you actually need, I think.

@Shatur
Copy link
Contributor Author

Shatur commented Jun 27, 2023

Makes sense, will do!

@Shatur
Copy link
Contributor Author

Shatur commented Jun 27, 2023

But directly deriving might not be desirable since it will add a Clone bound for every generic parameter.

But we already have Clone for RowIter and ColumnIter.
Maybe do the same for MatrixIter by adding additional parameter to the iterator! macro?

@Andlon
Copy link
Sponsor Collaborator

Andlon commented Jun 27, 2023

But we already have Clone for RowIter and ColumnIter.

Ah, I didn't realize that. That's quite unfortunate, and that's essentially a mistake. We should rather fix those Clone impls than perpetuate the same mistake for MatrixIter IMO.

@Shatur
Copy link
Contributor Author

Shatur commented Jun 27, 2023

Are you sure that it's a mistake? What issues current Clone derive causes?

@sebcrozet
Copy link
Member

sebcrozet commented Jul 8, 2023

Directly deriving Clone sounds fine. All the other type parameters already implement Clone anyway, and T: Scalar already implies T: Clone. So I would be in favor of keeping the clone derive, and relaxing the requirement late if we actually find a use-case that doesn’t fit.

But, yeah, Clone should not be implemented for MatrixIterMut.

@Shatur
Copy link
Contributor Author

Shatur commented Jul 8, 2023

@sebcrozet thanks for letting me know!

Should I add additional parameter to iterator! and pass Clone, Debug for MatrixIter and just Debug for MatrixIterMut?

@sebcrozet
Copy link
Member

@Shatur Yeah, I think that would make sense.

@Shatur
Copy link
Contributor Author

Shatur commented Jul 8, 2023

@sebcrozet done!

@sebcrozet sebcrozet merged commit e3443ca into dimforge:dev Jul 8, 2023
12 checks passed
@sebcrozet
Copy link
Member

Thanks!

@Shatur Shatur deleted the clone-iter branch July 8, 2023 16:31
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.

None yet

3 participants