Skip to content

Remove unsafe from array_ref! macro #24

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nabilwadih
Copy link

unsafe is no longer needed here since this is now supported in stable rust, the panic behavior is the same as before since it would panic anyways if the slice index was out of bounds

@burdges
Copy link
Contributor

burdges commented Jan 18, 2023

Anyone cleaver regression tests done yet?

If it's slower in niche cases, then maybe one should've a no_unsafe default feature doing this, so then basically everyone benefits but in theory someone who notices a regression could revert to the unsafe form. Just fyi, you can also prove unsafe code correct, ala the rustbelt project, which becomes basically as good as not having unsafe code.

@nabilwadih
Copy link
Author

@burdges thanks for the suggestion! I wrote a simple benchmark here: https://github.com/nabilwadih/arrayref/pull/1/files (open to suggestions on anything else you would like to see benchmarked)
And based on the results, there seems to be no difference in performance of the implementation without unsafe:
Screenshot 2023-01-18 at 10 50 41 AM
Based on that, I don't think it makes sense to add a no_unsafe default feature flag

@ejpcmac
Copy link

ejpcmac commented Mar 20, 2024

It seems removing the unsafe block has quite a big impact in term of size: on a thumv8m.main-none-eabihf target, I’m getting 128 bytes of additional usage per use of the macro when trying the code from this PR.

I’ve come to discover this wonderful crate because I was trying to find a way to split an array of u8 to u32 so I can feed them into registers, and found the conventional copy_from_slice or try_into methods that we see advertised everywhere introduce quite a big size penalty. I’ve been able to reduce the code size of a lib by ~10% just by using arrayref, so I’d be in favor to add a feature flag to keep the same size performance (maybe it could be non-default, so that only people needing an extra size optimisation would enable it).

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.

3 participants