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

Safety + Miri #14

Merged
merged 3 commits into from
Dec 13, 2022
Merged

Safety + Miri #14

merged 3 commits into from
Dec 13, 2022

Conversation

Wicpar
Copy link
Contributor

@Wicpar Wicpar commented Dec 12, 2022

  • removed all unchecked variants as they were slower than the new ones (~10-30%), and unsafe for no reason (~10-30%)
  • replaced most code with safe functions
  • Miri now passes all checks
  • disabled env logger for miri as it is unable to introspect.
  • replaced indices, ranges and sizes with usize, converting from u8 is not free, and there is no reason to use a u8 as it takes a full register anyway. Ranges are checked anyway too. This is also for API coherence with the rest of the rust ecosystem. Ranges are checked anyway too.

@Wicpar
Copy link
Contributor Author

Wicpar commented Dec 13, 2022

there is a good case to be made to use the replace_range function to solve insert, push, remove and their variations

Copy link
Owner

@paulocsanz paulocsanz left a comment

Choose a reason for hiding this comment

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

Wow, this is so much cleaner and safer! Thank you!

I will have to test this as we don't currently have CI, but it seems incredible. I will do it after work.

Don't mind my comments too much, I can make those changes later, although I was curious about changing &str for &[u8] in places where we expect utf8.

Thanks again, this is super awesome, you improved this library so much!

src/arraystring.rs Show resolved Hide resolved
src/utils.rs Show resolved Hide resolved
src/utils.rs Show resolved Hide resolved
src/arraystring.rs Show resolved Hide resolved
src/arraystring.rs Outdated Show resolved Hide resolved
src/arraystring.rs Show resolved Hide resolved
src/arraystring.rs Outdated Show resolved Hide resolved
@paulocsanz paulocsanz merged commit 670054f into paulocsanz:main Dec 13, 2022
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