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

Add Send + Sync markers for AlignedVec and Plan. #106

Merged
merged 2 commits into from
Feb 23, 2021

Conversation

richard-uk1
Copy link
Contributor

AlignedVec is definitely safe to be Send and Sync as it is mutable only behind unique mut pointers.

It is harder to reason about Plan being Sync because we don't know if it uses any UnsafeCell-like behaviour internally in fftw. However we can be fairly sure it is Send, as it will be used in situations that rely on using it in different threads (e.g. in a jack realtime thread).

Using this crate with jack is why I need the changes in this PR to use the library.

@richard-uk1
Copy link
Contributor Author

friendly ping

@TallJimbo
Copy link

I saw this while contemplating opening my own PR (or at least an issue) to make the Plan execute operations take non-mut references to self, because the FFTW docs are pretty clear about plan execution being thread-safe (and not modifying any internal data) as long as the new-array interfaces are used (which is what the safe Rust interface always does). Unless there's some subtlety I've missed, I think that means Plan can be sync, too.

@hombit
Copy link
Contributor

hombit commented Feb 23, 2021

@termoshtt, can you merge it, please?

@termoshtt termoshtt merged commit 17d48b5 into rust-math:master Feb 23, 2021
termoshtt added a commit that referenced this pull request Feb 23, 2021
@hombit
Copy link
Contributor

hombit commented Feb 23, 2021

Thank you!

@termoshtt
Copy link
Member

https://github.com/rust-math/fftw/releases/tag/fftw-0.7.0 released with #109

@richard-uk1 richard-uk1 deleted the send_sync branch February 23, 2021 16:49
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.

4 participants