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

Adding into_raw() and from_raw() methods #29

Merged
merged 1 commit into from
Sep 14, 2023

Conversation

bendk
Copy link
Contributor

@bendk bendk commented Aug 29, 2023

See issue #28.

@bendk
Copy link
Contributor Author

bendk commented Aug 29, 2023

Created a PR to continue the discussion from #28. What do you think? I considered adding tests, but I couldn't think of a meaningful way to do it.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Copy link
Owner

@faern faern left a comment

Choose a reason for hiding this comment

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

I'd love to see some test, even if it could be hard to find very meaningful ones. At least tests that does the following would be good:

  1. Sends on a channel where the Receiver is currently in the "raw state" and verify it can be reconstructed and the message received afterwards.
  2. That a sender can be converted to raw and back and still successfully send a message on the channel.

@bendk bendk force-pushed the into-raw-from-raw branch from 0af70cd to df1b3a2 Compare August 30, 2023 15:27
@bendk
Copy link
Contributor Author

bendk commented Aug 30, 2023

Updated the docs and added tests. It's a good thing you suggested that, because I forget to call std::mem::forget in the initial code.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@bendk bendk force-pushed the into-raw-from-raw branch from df1b3a2 to 939f59e Compare September 5, 2023 14:26
@bendk
Copy link
Contributor Author

bendk commented Sep 5, 2023

Thanks for working through this one with me. I moved the test into the tests directory and the docstrings are looking great to me. Did I miss anything?

For what it's worth: I don't think we're actually going to need this, but I still think the functionality could be useful to someone else (and maybe us in the future).

Copy link
Owner

@faern faern left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! I'm going to merge this in its current state.

One thing I'm not 100% satisfied with is the pointer type: *mut (). It's completely untyped and hard to understand. Somewhat of a footgun to accidentally recreate a Sender/Receiver of the wrong type. I'm not sure what would be better. But I'm considering either returning *mut T where T is the type in the channel. I know this is wrong (but so is *mut ()), but at least it contains type information. OR create some special public type in the library such as RawChannel<T> and return *mut RawChannel<T>. I'm not sure if this actually has any practical benefit at all. Most users will probably cast this to a void pointer when going through FFI anyway. Would *mut core::ffi::c_void be better? Or is it then too C specific?

@faern
Copy link
Owner

faern commented Sep 6, 2023

Oh yeah. I won't merge right now. The CI don't pass. You will have to look into that.

@bendk bendk force-pushed the into-raw-from-raw branch from 939f59e to 6c1b6b3 Compare September 6, 2023 20:40
@bendk
Copy link
Contributor Author

bendk commented Sep 6, 2023

I think the CI issues should be fixed.

The pointer type question is a hard one. *mut () doesn't seem perfect, but Rust does have a convention of using () to represent untyped pointers (https://doc.rust-lang.org/std/task/struct.RawWaker.html). I think I prefer that over c_void.

What about making the Channel struct public, but none of its fields or methods? That would allow returning * Channel<T>, which actually is the correct pointer type.

In the end though, I think you're right that users are going to end up casting it to so maybe it doesn't matter too much.

@faern
Copy link
Owner

faern commented Sep 7, 2023

but Rust does have a convention of using () to represent untyped pointers

Awesome. Examples of this being used by std was just what I needed to be convinced. Let's keep it at *mut ()

I think the CI issues should be fixed.

Sadly not. It's still missing on some combinations of features. You either need to use try_recv in the tests or cfg out the tests when certain features are off. I'd vote for the former. None of your tests should need to block.

@bendk bendk force-pushed the into-raw-from-raw branch from 6c1b6b3 to 711f956 Compare September 7, 2023 13:08
@bendk
Copy link
Contributor Author

bendk commented Sep 7, 2023

try_recv works for me, I just updated the tests to do that. Tell me if there's a better way to handle this CI feedback loop.

@faern
Copy link
Owner

faern commented Sep 8, 2023

Not sure why the waker size tests fail. I will try to look into that in the weekend.

@bendk
Copy link
Contributor Author

bendk commented Sep 8, 2023

Not sure why the waker size tests fail. I will try to look into that in the weekend.

Those fail for me as well when I run cargo test locally. My guess was that newer versions of Rust are optimizing the struct better so that the padding for one field is used for part of another field or something like that.

@faern
Copy link
Owner

faern commented Sep 9, 2023

Apparently the size of the ReceiverWaker went from 24 to 16 bytes in Rust 1.65. I can't find anything in the release notes that explains why. The size of std::task::Waker is 16 bytes in both older and newer Rust. But I guess some internal change made it aware of invalid representations, that it could then optimize with the help of, to fit the ReceiverWaker enum discriminant into that space.

I have now pushed fixes to this to main. Can you please rebase and push your code again?

@bendk
Copy link
Contributor Author

bendk commented Sep 11, 2023

Sure. The tests are now passing locally for me as well.

@faern faern merged commit 8c4d1da into faern:main Sep 14, 2023
6 of 11 checks passed
@faern
Copy link
Owner

faern commented Sep 14, 2023

Thank you for the contribution! Fixing the broken CI took a few days since I temporarily did not have access to my GPG key, and I wanted to sign the commits. But now it's fixed, and your PR is merged. Hopefully this is helpful to someone :)

@bendk
Copy link
Contributor Author

bendk commented Sep 14, 2023

Awesome, thanks for working with me on this one.

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