Skip to content

impl Clone where appropriate #43

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

Merged
merged 1 commit into from
Oct 9, 2018
Merged

Conversation

strake
Copy link
Member

@strake strake commented Oct 8, 2018

I'm not sure what your criteria are for what should be Clone and/or Copy, so let me know what you think of it. (I am using the impl for Port in particular.)

@phil-opp
Copy link
Member

phil-opp commented Oct 8, 2018

Thanks for the PR!

I don't think that it's a good idea to implement Copy for all possible types, because it is often not necessary and might hinder us from extending the type later (removing the Copy implementation later would be a breaking change). So I would only implement Copy if there is some reason to do so. I don't see such a reason for the various error structs or the Cr0/Cr3 registers (there's nothing you can do with a Cr0 instance).

Another disadvantage when implementing Copy is that it is possible to accidentally copy something instead of borrowing it, so that instead of modifying the original object, the modification happens on a copy and is lost. Therefore I would refrain from implementing Copy for everything with modifiable inner state (such as GlobalDescriptorTable). Instead, I would only implement Clone, so that the copying is explicit and can't happen accidentally.

So instead of implementing Copy wherever possible, I would only implement it when it's needed and doesn't lead to accidental mistakes. Deriving Clone, PartialEq, and Eq for Port seems fine to me and deriving Clone (but not Copy) for GlobalDescriptorTable, Descriptor, InterruptDescriptorTable, and ExceptionStackFrame seems fine to me too. Are there other Clone/Copy implementations that you need?

@strake
Copy link
Member Author

strake commented Oct 8, 2018

Makes sense. (The Eq and PartialEq impls slipped in while i was staging the PR, but i do want them.) I mostly need the impls for Port right now, but thought i ought to figure out what else ought to be Clone while i'm at it.

PTAL

@phil-opp phil-opp changed the title impl {Clone, Copy} where appropriate impl Clone where appropriate Oct 9, 2018
@phil-opp
Copy link
Member

phil-opp commented Oct 9, 2018

Thanks for the update! Looks good to me.

@phil-opp phil-opp merged commit 9de5326 into rust-osdev:master Oct 9, 2018
@strake strake deleted the clone branch October 9, 2018 11:06
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.

2 participants