Document safety in memchr, remove unsafe. #76488
Conversation
Documents the safety of two unsafe blocks and removes an unnecessary one
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
jyn514
added
T-libs
| } else { | ||
| let (lower, rest) = text.split_at(offset); | ||
| (lower.len(), len - rest.align_to_offsets::<(Chunk, Chunk)>().1) | ||
| } |
There was a problem hiding this comment.
I think the previous code is better in this case. Even though it's unsafe, it is much clearer (IMO) than this replication of the body of align_to.
There was a problem hiding this comment.
@Mark-Simulacrum One of the main reasons I removed it is because I was having some trouble figuring out the best way to document it. Alongside that, it wasn't immediately clear to me what align_to did, and why it was being used instead of some other more specialized function (which doesn't exist, in fact. shame.)
There was a problem hiding this comment.
Hm, so the documentation on align_to is fairly clear to my eyes, but if you're not understanding it then we should definitely improve it! It's sort of a "core primitive" in my eyes for this sort of task. In the case of &[u8] and usize as the middle type, align_to is also safe in all cases, because transmutes of byte slices to slices of usize, when respecting alignment (and length), are always safe.
Mark-Simulacrum
added
S-waiting-on-author
|
It looks like another (closer to what I would prefer) version is up here: #76688 (comment) Up to you, but we can close this - I'm happy to merge whichever. |
|
This should be ready, though if you prefer the one with no unsafe removed, that's fine. |
|
I disagree that "one less unsafe" outweighs the costs of the non-trivial inlined algorithm from align_to. Once safe transmute work progress I imagine we'll probably think about adding align_to_safe (probably with a different name) that could be used here. |
|
Ping from triage: |
|
Ah, shoot. Sorry for forgetting about the PR. I'll make requested changes when I have time. |
|
☔ The latest upstream changes (presumably #76971) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels: |
|
time has not been in my favor. |
Documents the safety of two unsafe blocks and removes an unnecessary one.
I kindly ask someone take a closer look at lines 100..106 to make sure I didn't calculate the aligned offsets wrong. I ran the test suite with and without an alignment assert included so they should be correct.