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

Remove unsafe #47

Merged
merged 3 commits into from
Dec 30, 2023
Merged

Remove unsafe #47

merged 3 commits into from
Dec 30, 2023

Conversation

BillyDM
Copy link

@BillyDM BillyDM commented Dec 29, 2023

I've been meaning to do this for a while, but I'm finally getting around to it.

I removed all uses of unsafe Rust, and there is no meaningful impact on performance. While the unsafe stuff was probably sound, I would much prefer to not have any unsafe if we can.

The main reason I used unsafe before was to avoid needing to zero out buffers before sending them to the decoder to fill. But I figured out I can get around this by just clearing the Vecs and using extend_from_slice() inside the decoder instead.

Although of course removing the unsafe is a breaking change, but I don't think there's anyone that was creating their own custom decoders and encoders anyway (if I'm wrong, please correct me). I think it's okay to just bump the version of creek to 1.2.0.


I also bumped the dependency versions since that was simple to do.


I also updated the demos to use the latest version of egui. I would have done this in a separate PR, but I wasn't able to get the old demos to run, and I wanted to use them to test if my changes still worked as expected.

@BillyDM BillyDM requested review from orottier and uklotzde December 29, 2023 16:29
Copy link
Collaborator

@orottier orottier left a comment

Choose a reason for hiding this comment

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

Great to hear you were able to remove all unsafe without any real drawbacks. forbid_unsafe will definitely help getting trust of downstream users.

I only skimmed through the changes, but they look very reasonable to me. All demos work for me as well.

Changing a trait signature is a breaking change (read the final note over at https://doc.rust-lang.org/cargo/reference/semver.html#fn-unsafe-safe ) but it does seem a bit excessive indeed to increase major version. I'm no purist so v1.2 is fine with me.

@BillyDM
Copy link
Author

BillyDM commented Dec 30, 2023

Cool. I'll go ahead a merge this then.

@BillyDM BillyDM merged commit 643f58b into main Dec 30, 2023
4 checks passed
@BillyDM BillyDM deleted the remove_unsafe branch January 2, 2024 18:52
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