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
add a panic example to std::from_utf8_unchecked #35890
add a panic example to std::from_utf8_unchecked #35890
Conversation
/// | ||
/// Incorrect bytes: | ||
/// | ||
/// ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be no_run
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this example won't panic, so I guess it won't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it might? The from_utf8_unchecked
won't fail, but printing it might? Not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example won't panic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I understand, this function should never panic (hence it being "unchecked").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unchecked
means it won't return an Err
. It won't panic either, but the call to println!
will, as it attempts to print an invalid string. I'll split the lines out to make that clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, never mind. It won't panic there either.
While I can't think of any specific examples right now, it'd be great if we could somehow demonstrate memory safety issues, as described in the "Safety" section of this function. |
@frewsxcv Yeah, I can't think of any examples either. If somebody reading this knows of one, can you let me know? Thanks in advance! |
@ubsan ping! |
@matthew-piziak There are no memory safety issues with the current runtime. Someone could write something assuming valid utf-8, however, and read right off the end, however, for example with a |
@ubsan Gotcha, thanks. What do you think of this PR as-is, then? |
/// use std::str; | ||
/// | ||
/// // some invalid bytes, in a vector | ||
/// let sparkle_heart = vec![0, 159, 146, 150]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe change this name, since it's no longer a sparkle heart?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I also renamed the example in the checked conversion.
@matthew-piziak I don't generally recommend committing UB in documentation, no matter how frivolous I may think that UB is :P. I'm not sure... You could say "poses a safety issue" as well. |
@ubsan Even with |
@matthew-piziak I meant more of "don't show people UB in documentation". I'd rather use correct examples. |
@ubsan That's fair. Shall we close this PR? |
Yes, I agree. Thanks @matthew-piziak ! |
Decided not to show undefined behavior in documentation. |
Show that passing invalid bytes to
str::from_utf8_unchecked
is a runtime panic.r? @steveklabnik