Skip to content

automata: remove some unsafe code#1189

Closed
joshlf wants to merge 1 commit into
rust-lang:masterfrom
joshlf:remove-unsafe
Closed

automata: remove some unsafe code#1189
joshlf wants to merge 1 commit into
rust-lang:masterfrom
joshlf:remove-unsafe

Conversation

@joshlf
Copy link
Copy Markdown

@joshlf joshlf commented May 4, 2024

Note that the changes in wire.rs require zerocopy's "derive" feature, but the changes in accel.rs do not. If you'd prefer not to take a dependency on a proc macro derive, we can still salvage some of this PR.

@BurntSushi
Copy link
Copy Markdown
Member

I was aware of zerocopy (and similar crates) when I wrote this code, and knew that it could probably eliminate some unsafe blocks. But as a rule of thumb, I don't add dependencies to regex. I have spent a lot of time and effort to keep its dependency tree small and reasonable (the entire tree is owned by me for example). So not only does taking a proc macro dependency not meet my bar, zerocopy itself doesn't either. Or at least, it would need to provide a much bigger benefit.

The way to get rid of these unsafe blocks is to push on the "safe transmute" effort I think.

@joshlf
Copy link
Copy Markdown
Author

joshlf commented May 4, 2024

Yeah no worries, I understand the desire to avoid dependencies. We do the same. Happy to just close this if you'd prefer.

As for safe transmute, IIUC (@jswrenn is more directly involved and can provide more context), the idea is that safe transmute will basically just enable a trait bound, and you'll still need library code to provide useful abstractions given that bound. Roughly, safe transmute will replace zerocopy-derive, but not zerocopy (and presumably the same for bytemuck/bytemuck-derive, etc), and code like this PR would still require zerocopy in order to avoid writing unsafe.

@BurntSushi
Copy link
Copy Markdown
Member

Aye. Understood. Thanks for the attempt! I do overall support trying to remove unsafe blocks, but probably not with the addition of other dependencies.

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