Skip to content

Require all SPI and Serial word types to be Copy #326

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
Jan 5, 2022

Conversation

GrantM11235
Copy link
Contributor

The word type should always be a builtin integer (u8, u16, etc) or a newtype to represent an integer with an unusual size, which will almost certainly be Copy.

I also removed the 'static bound from the spi transactional stuff because it appears to be unnecessarily now.

@GrantM11235 GrantM11235 requested a review from a team as a code owner November 16, 2021 20:04
@rust-highfive
Copy link

r? @ryankurte

(rust-highfive has picked a reviewer for you, use r? to override)

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Requiring Copy for word types seems fine to me.
I am not sure what the reason for the 'static bound was so I will leave that to @ryankurte

@eldruin eldruin added this to the v1.0.0 milestone Nov 19, 2021
@Dirbaio
Copy link
Member

Dirbaio commented Nov 19, 2021

Perhaps we want to require 'static for all words? I can't imagine a legit reason for it not to be static

@ryankurte
Copy link
Contributor

ryankurte commented Nov 19, 2021

i had to go digging [1, 2], it appears the justification was to separate the lifetime of the type W from that of the buffers &'a [W], i can't imagine i would have added it if unnecessary, but, if the compiler can infer the lifetime of the word type now perhaps that is no longer required.

Copy sgtm too. there aren't a lot of reasons for W not to be static, but, it's also seemingly an unnecessary bound so, provided it is demonstrated to still work on both sides i'd be okay with removing the 'static.

@GrantM11235
Copy link
Contributor Author

I decided to leave the 'static bound unchanged for now because I don't fully understand it, and it seems to be unrelated to this PR.

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!
bors r+

@bors bors bot merged commit de3edb0 into rust-embedded:master Jan 5, 2022
@GrantM11235 GrantM11235 deleted the word-copy branch January 9, 2022 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Review is incomplete T-hal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants