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

Require Send for T in Queue instead of Copy #75

Merged
merged 1 commit into from
Oct 16, 2024

Conversation

eivindbergem
Copy link
Contributor

By requiring items to be Copy we reduce the queue to only work with primitives. But, queue dosn't require Copy to send items. Yes, they are technically copied over to the Queue buffer, but semantically this is a move, and having send() consume it's argument provides the move semantics.

The PR removes the Copy bound from Queue.

@eivindbergem
Copy link
Contributor Author

I jumped the gun a bit on this. There are some remaining issues:

  • The item has to be forgotten to avoid calling drop
  • The item must be Send

@eivindbergem
Copy link
Contributor Author

Fixed the above point. Also, in receive, use MaybeUninit to safely deal with uninitialized memory and avoid calling drop on it in case of timeout.

@eivindbergem eivindbergem changed the title Queue shouldn't require Copy Require Send for T in Queue instead of Copy Aug 23, 2024
@schteve
Copy link
Collaborator

schteve commented Oct 3, 2024

Sorry for the delay in getting to this! I think it's a great point and it makes sense that if you're giving a value to the queue that it could be owned by the queue.
However, after looking it over I had a few questions. Maybe you know?

  1. In the places where mem::forget() is used, it happens before copying the value into the queue. I think this is wrong as the memory may no longer be valid (forget() consumes the value after all - "However, it does not guarantee that pointers to this memory will remain valid.". Then again I'm not sure if it's technically correct to allow the queue to copy out the value before forgetting it either - is it OK to have two copies of the data even if for just a moment?
  2. What does it make sense to do with values that aren't accepted by the queue? It seems a bit odd to just consume the value regardless. Maybe it should be returned with the Err case? Or maybe dropped? With Copy I think we don't have to worry about this.
  3. Using MaybeUninit seems like a great way to avoid needing to manage drop. Would it make sense to use MaybeUninit::uninit() rather than zeroed() though just to avoid the needless overhead of zeroing?

@eivindbergem
Copy link
Contributor Author

Sorry for the delay in getting to this! I think it's a great point and it makes sense that if you're giving a value to the queue that it could be owned by the queue. However, after looking it over I had a few questions. Maybe you know?

1. In the places where `mem::forget()` is used, it happens before copying the value into the queue. I think this is wrong as the memory may no longer be valid (`forget()` consumes the value after all - "[However, it does not guarantee that pointers to this memory will remain valid.](https://doc.rust-lang.org/beta/core/mem/fn.forget.html)". Then again I'm not sure if it's technically correct to allow the queue to copy out the value before forgetting it either - is it OK to have two copies of the data even if for just a moment?

I think you're right on this. In theory, the compiler could use the memory for something else after the call to forget().

2. What does it make sense to do with values that aren't accepted by the queue? It seems a bit odd to just consume the value regardless. Maybe it should be returned with the Err case? Or maybe dropped? With Copy I think we don't have to worry about this.

Returning it with the Err case makes the most sense to me.

3. Using `MaybeUninit` seems like a great way to avoid needing to manage drop. Would it make sense to use `MaybeUninit::uninit()` rather than `zeroed()` though just to avoid the needless overhead of zeroing?

Yes, no need to zero it out.

By requiring items to be `Copy` we reduce the queue to only work with
primitives. What is more appropriate here is to require that the item
is `Send` and use move semantics instead of copy.
@eivindbergem
Copy link
Contributor Author

Using ManuallyDrop instead of mem::forget().

Had to add new error type to return item in error case. I just propagate the FreeRtosError in processor.rs for now, but I can add proper error handling here as well if that's wanted.

@schteve
Copy link
Collaborator

schteve commented Oct 16, 2024

LGTM, thanks! Good call on ManuallyDrop - leaving this link for future reference.

@schteve schteve merged commit 21b753c into lobaro:master Oct 16, 2024
2 of 3 checks passed
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