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 Copy, Clone for all types in common.rs #81

Merged

Conversation

LinusCDE
Copy link
Collaborator

@LinusCDE LinusCDE commented Dec 10, 2021

This is a minor fix that allows to Copy and Clone all types in src/framebuffer/common.rs. More a minor thing to improve usability for some cases.

I originally stumbled upon this when doing doomarkable. Wanted to specify the waveform depending on the device and later reuse it depending on the model in a render loop. Problem was that the variable in loop wouldn't work, since the parameter is owned and rusts ownership wouldn't permit me to give ownership repeatedly away in a loop (duh).

Wouldn't be a problem if the waveform mode could be cloned or such, but this wasn't derive. So I would basically need to create my own clone, remember it otherwise (basically serializing to a different type (e.g. and int) deserializing it again every time). My workaround was to just find out the same value every time in the loop while it would be perfectly fine to do once before the loop (any maybe use a clone on that value if not also Copy).

Clone would be enough, but those types are pretty primitive and surely not worth the cloning. Also more complex types in the same file already had also "Copy" specifies. So I took the liberty to ensure that all types in that file have Copy and Clone.

For the next version, looking that everything is at least Debug'able and (in most cases) at Clone'able would probably be good as well. Just wanted to to this quick fix now since that note sat on my todo list ever since.

Maybe those types should also get PartialEq, Eq and Hash as well in case people would need it. Not sure what Rust's philosophy would be in such a case (better to "over-derive" or "under-derive"?).

@fenollp
Copy link
Collaborator

fenollp commented Dec 10, 2021

I don't have an opinion. I'm fine with the changes. Usually I'm in the team "let's add things as we need them" and not "we might need this in the future so let's add them now".

@LinusCDE
Copy link
Collaborator Author

Fine with me as well. I think these changes should suffice for the foreseeable future.

@fenollp fenollp merged commit bbfa73a into canselcik:master Dec 11, 2021
@LinusCDE LinusCDE added this to the 0.6.0 milestone Jan 17, 2022
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

2 participants