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

Override clone_from implementations for a few types #2236

Merged
merged 1 commit into from
May 20, 2024

Conversation

a1phyr
Copy link
Contributor

@a1phyr a1phyr commented May 16, 2024

I license past and future contributions under the dual MIT/Apache-2.0 license,
allowing licensees to choose either at their option.

@fintelia
Copy link
Contributor

How common is manually implementing clone_from? I don't think I've seen it done in other crates

Copy link
Member

@HeroicKatora HeroicKatora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's odd that derive() does not do it, but it obviously has the potential to save an allocation. Not quite as intricate as std::vec::Vec on the dynamic image but effective.

@fintelia
Copy link
Contributor

This would avoid the allocation but potentially "leak" memory if the image started out substantially larger than the new size. (Technically not a leak because the memory would still remain referenced but inaccessible.) Though perhaps that's well known behavior and folks using the method would expect that?

@kornelski
Copy link
Contributor

I think it's fine that clone_from keeps potentially large allocations. It's opt-in, and its purpose is to reuse allocations.

src/dynimage.rs Outdated Show resolved Hide resolved
src/dynimage.rs Outdated
(Self::ImageRgba16(p1), Self::ImageRgba16(p2)) => p1.clone_from(p2),
(Self::ImageRgb32F(p1), Self::ImageRgb32F(p2)) => p1.clone_from(p2),
(Self::ImageRgba32F(p1), Self::ImageRgba32F(p2)) => p1.clone_from(p2),
(this, source) => *this = source.clone(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This match arm should be avoided. The enum is marked non-exhaustive, but within this crate we can still match exhaustively on it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure of what you want here. Something like this ?

match (self, source) {
    (Self::ImageRgb8(p1), Self::ImageRgb8(p2)) => p1.clone_from(p2),
    (Self::ImageRgba8(p1), Self::ImageRgba8(p2)) => p1.clone_from(p2),
    // other formats...

    (this, Self::ImageRgb8(p)) => *this = Self::ImageRgb8(p.clone()),
    (this, Self::ImageRgba8(p)) => this = Self::ImageRgba8(p.clone()),
    // other formats...
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't read closely. It's fine as is

@fintelia fintelia merged commit d48c6a6 into image-rs:main May 20, 2024
31 checks passed
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