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 Send and Sync for Controller. #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

adeschamps
Copy link
Contributor

See #6

I believe these are safe to implement because of the reasons I describe in the comments of the impl blocks.

@JMurph2015
Copy link
Collaborator

I think we'd have to take a harder look at the C library before implementing Sync. I think Send is probably fine, but it's not actually even totally clear that Send is fair because the run-time initialization of the LED strip arrays in the ws2811_t structs could end up putting references to the same region of memory in those structs.

@JMurph2015
Copy link
Collaborator

Update: Reading a little more about the Sync trait in particular, I think that is viable, but I still don't know about Send for the Controller struct itself.

@adeschamps
Copy link
Contributor Author

I'm taking another look at it, and in its current state Sync is not safe to implement because Controller also implements Clone - that would lead to pointer aliasing. Would it be possible to remove the Clone impl?

@JMurph2015
Copy link
Collaborator

Yeah I can't come up with a good reason it should implement Clone, however, I also don't have a great reason to believe it should implement Sync.

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