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

Why so many usafe unchecked functions ? #6

Closed
Wicpar opened this issue Dec 9, 2022 · 8 comments
Closed

Why so many usafe unchecked functions ? #6

Wicpar opened this issue Dec 9, 2022 · 8 comments

Comments

@Wicpar
Copy link
Contributor

Wicpar commented Dec 9, 2022

lots of unchecked functions check things anyway, it makes no sense to have an unsafe version... Can i remove them while i do a refactoring for #5 ?

@paulocsanz
Copy link
Owner

If we can remove unsafety without affecting performance then it's a no-brainer.

If there is a performance issue in production then we should review on a case by case basis.

If you are talking about providing unchecked functions to the user, then it's a different thing. This issue was kinda confusing so I'm not sure I understand what you mean exactly.

@Wicpar
Copy link
Contributor Author

Wicpar commented Dec 9, 2022

for instance push_str_unchecked does all the work necessary for try_push_str if we move around a few of the calculations, the return of a result adds negligible overhead, and will be optimized out if used as the basis for push_str, that simply truncates the input and unwraps the result.

@Wicpar
Copy link
Contributor Author

Wicpar commented Dec 9, 2022

as you can see here it produces even better assembly

@Wicpar
Copy link
Contributor Author

Wicpar commented Dec 9, 2022

also it looks like this could use a maximum of 4 iterations explicitly, generated code unrolls way too far.

// in truncate_str
while !slice.is_char_boundary(index) {
    index = index.saturating_sub(1);
}

@paulocsanz
Copy link
Owner

Nice, if you could provide a PR that would be great, it can be in the same PR but then it will be a bit harder to review. I currently don't have that much free time and don't want to delay your work getting in.

@Wicpar
Copy link
Contributor Author

Wicpar commented Dec 9, 2022

just tell me if you want to go forward with the #7 style or not, i will change them in a separate PR for now

@paulocsanz
Copy link
Owner

I need some time to review it, I am at work right now and have weekend plans that may prevent me from reviewing, but I will as soon as I can

@paulocsanz
Copy link
Owner

Fixed on main!

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

No branches or pull requests

2 participants