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

Implement Send for chunk types #78

Merged
merged 2 commits into from
Mar 3, 2022
Merged

Conversation

RamType0
Copy link
Contributor

#4

@RamType0
Copy link
Contributor Author

This clippy error is really weird

error: this implementation is unsound, as some fields in Producer<T> are !Send
--> src/lib.rs:291:1
|
291 | unsafe impl<T: Send> Send for Producer {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: -D clippy::non-send-fields-in-send-ty implied by -D warnings
note: the type of field buffer is !Send
--> src/lib.rs:278:5
|
278 | buffer: Arc<RingBuffer>,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
= help: add bounds on type parameter T that satisfy Arc<RingBuffer<T>>: Send
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_send_fields_in_send_ty

error: this implementation is unsound, as some fields in Consumer<T> are !Send
--> src/lib.rs:496:1
|
496 | unsafe impl<T: Send> Send for Consumer {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: the type of field buffer is !Send
--> src/lib.rs:483:5
|
483 | buffer: Arc<RingBuffer>,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
= help: add bounds on type parameter T that satisfy Arc<RingBuffer<T>>: Send
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_send_fields_in_send_ty

error: could not compile rtrb due to 2 previous errors
warning: build failed, waiting for other jobs to finish...
error: build failed
Error: Process completed with exit code 101.

rust-lang/rust-clippy#8045

@mgeier
Copy link
Owner

mgeier commented Jan 18, 2022

Yeah, that's strange.
AFAIU, this lint will be disabled in the next point release which comes out within the next few days.

Interestingly, this lint is not triggered by #75.

@mgeier
Copy link
Owner

mgeier commented Jan 22, 2022

Rust 1.58.1 has been released, I've restarted CI and now it passes.

@@ -444,6 +444,8 @@ pub struct WriteChunkUninit<'a, T> {
producer: &'a Producer<T>,
}

unsafe impl<T: Send> Send for WriteChunkUninit<'_, T> {}
Copy link
Owner

Choose a reason for hiding this comment

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

What about adding a comment regarding safety?

Maybe something like this:

// WriteChunkUninit only exists while a unique reference to the Producer is held.
// It is therefore safe to move it to another thread.

Copy link
Owner

@mgeier mgeier left a comment

Choose a reason for hiding this comment

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

I've added comments as suggestions, what do you think about them?

src/chunks.rs Show resolved Hide resolved
src/chunks.rs Show resolved Hide resolved
@mgeier mgeier merged commit 7279fc1 into mgeier:main Mar 3, 2022
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