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

Impl Interpolator for &mut (impl Iterator). #104

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Impl Interpolator for &mut (impl Iterator). #104

wants to merge 1 commit into from

Conversation

quadrupleslap
Copy link

Wouldn't you usually want to keep the state across interpolate calls?

This isn't a breaking change afaik.

@andrewcsmith
Copy link
Contributor

Yeah, right now next_source_frame only takes &mut self so it does actually keep state across calls when it's used in the Converter. Converter currently owns the Interpolator, but borrows it for each successive call.

Instead, maybe the advantage of this is that it would let you use the same backing buffer for multiple Converter objects in succession, provided that only one Converter is alive at a time. In other words, it can save some memory allocations in very special circumstances at runtime, which is always a plus.

Anyway, this could technically be a breaking change if a downstream crate has an overlapping implementation, and you'd have the conflicting impls error. IDK where that falls on the semver hierarchy, but it's a perfectly wonderful contribution either way IMO. Thanks!

@mitchmindtree
Copy link
Member

Looks fine to add to me!

@andrewcsmith I don't think this is a breaking change as I don't think a user can implement Interpolator for &'a mut T (or any &'a mut <insert_type>) downstream due to rust's orphan rules which aim to prevent this problem (though I haven't actually checked and could be wrong! just going by memory).

For some reason travis failed on nightly, running the build again to make sure travis can finish successfully before merging.

@mitchmindtree
Copy link
Member

Ahh, looks like this is legitimately failing to compile in the --no-default-features build.

@micwoj92
Copy link

This branch has conflicts that must be resolved

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.

4 participants