Skip to content

WriteOnly shouldn't implement Clone #11

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

Closed
Freax13 opened this issue Jul 9, 2020 · 3 comments · Fixed by #12
Closed

WriteOnly shouldn't implement Clone #11

Freax13 opened this issue Jul 9, 2020 · 3 comments · Fixed by #12

Comments

@Freax13
Copy link
Member

Freax13 commented Jul 9, 2020

WriteOnly simply derives Clone so it calls Volatile::clone to clone its field,
https://github.com/embed-rs/volatile/blob/a5a6d786995df7cf07ed8689f79805da10ba2429/src/lib.rs#L210-L211
but Volatile::clone uses Volatile::read internally which might not work since the value is write-only.
https://github.com/embed-rs/volatile/blob/a5a6d786995df7cf07ed8689f79805da10ba2429/src/lib.rs#L144-L148

Removing Clone from WriteOnly is obviously a breaking change, so I'm not sure what to do about this.

@Freax13
Copy link
Member Author

Freax13 commented Jul 9, 2020

the same logic probably also applies to Debug

@Freax13 Freax13 changed the title WriteOnly shouldn't implement WriteOnly shouldn't implement Clone Jul 10, 2020
@phil-opp
Copy link
Member

Thanks for reporting! I would be happy to merge a pull request that fixes this. The breaking change isn't a problem since we can just release it as a new semver-incompatible version.

@oli-obk I'm thinking about moving this repo to the rust-osdev organization, would that be fine with you?

@oli-obk
Copy link

oli-obk commented Jul 29, 2020

I'm thinking about moving this repo to the rust-osdev organization, would that be fine with you?

yes

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 a pull request may close this issue.

3 participants