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.

Sign up for GitHub

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

Jump to bottom

Document unsafety in core::slice::memchr #76688

Merged
merged 1 commit into from
Nov 25, 2020
Merged

Document unsafety in core::slice::memchr #76688

merged 1 commit into from
Nov 25, 2020

Conversation

yokodake
Copy link
Contributor

Contributes to #66219

Note sure if that's good enough, especially for the align_to call.
The docs only mention transmuting and I don't think that everything related to reference lifetimes and state validity mentioned in the nomicon are relevant here.

@rust-highfive
Copy link
Collaborator

r? @KodrAus

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 13, 2020
@Mark-Simulacrum
Copy link
Member

It looks like CI is failing here, but this looks good to me. I think you may have raced with #76488, though. I'm happy to accept either, whichever one is ready first probably :)

@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Sep 14, 2020
Closed
@jyn514 jyn514 added T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Sep 15, 2020
@KodrAus
Copy link
Contributor

KodrAus commented Sep 22, 2020

Hi @yokodake 👋

It looks like we've got a few failing tidy checks on this one:

tidy error: /checkout/library/core/src/slice/memchr.rs:118: undocumented unsafe
tidy error: /checkout/library/core/src/slice/memchr.rs:119: trailing whitespace

@KodrAus
Copy link
Contributor

KodrAus commented Sep 22, 2020

@rustbot modify labels to +S-waiting-on-author and -S-waiting-on-review

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 22, 2020
@yokodake
Copy link
Contributor Author

Sorry didn't knew about these tidy checks.
fixed it

@bors
Copy link
Contributor

bors commented Oct 1, 2020

☔ 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:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 1, 2020
@KodrAus
Copy link
Contributor

KodrAus commented Oct 5, 2020

It looks like we've refactored our memchr implementation a bit, but I think these comments will still be valid.

@yokodake would you like to rebase your PR?

@KodrAus
Copy link
Contributor

KodrAus commented Oct 18, 2020

@rustbot modify labels: +S-waiting-on-author -S-waiting-on-review

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 18, 2020
@KodrAus
Copy link
Contributor

KodrAus commented Oct 18, 2020

Ah my bad! Looks like the rebase did happen 🙂

@rustbot modify labels: -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 18, 2020
@KodrAus
Copy link
Contributor

KodrAus commented Oct 18, 2020

This looks good to me! Thanks for working on it @yokodake!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 18, 2020

📌 Commit a506461 has been approved by KodrAus

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 18, 2020
@bors
Copy link
Contributor

bors commented Oct 18, 2020

⌛ Testing commit a506461 with merge ba432fcfbe3d11cf6a4ef766effdb5c7da044ad8...

@bors
Copy link
Contributor

bors commented Oct 18, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 18, 2020
@pietroalbini
Copy link
Member

@bors r-

The PR failed but bors forgot about that during synchronize.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 19, 2020
@yokodake
Copy link
Contributor Author

What kind of test could have failed, I only added comments.
The only thing I can see in the logs is completed with exit code 1.
Do I have to do another rebase? There doesn't seem to be any merge conflict though.

@oliviacrain
Copy link
Contributor

@yokodake: It seems like this build failure doesn't have anything to do with your PR- there was (is?) an intermittent issue with git on i686-mingw causing builds to fail (#73992). You might want to ping someone with bors privileges and get them to retry the build.

@oliviacrain oliviacrain mentioned this pull request Oct 24, 2020
Closed
@yokodake
Copy link
Contributor Author

ok, thanks.

@KodrAus

@rustbot modify labels: -S-waiting-on-review +S-waiting-on-author

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 13, 2020
@m-ou-se
Copy link
Member

m-ou-se commented Nov 13, 2020

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 13, 2020
@Dylan-DPC-zz
Copy link

@bors r=kodrAus

@bors
Copy link
Contributor

bors commented Nov 24, 2020

📌 Commit a506461 has been approved by kodrAus

sdroege
sdroege reviewed Nov 24, 2020
View reviewed changes
library/core/src/slice/memchr.rs
@@ -105,6 +105,8 @@ pub fn memrchr(x: u8, text: &[u8]) -> Option<usize> {
let (min_aligned_offset, max_aligned_offset) = {
// We call this just to obtain the length of the prefix and suffix.
// In the middle we always process two chunks at once.
// SAFETY: transmuting `[u8]` to `[usize]` is safe except for size differences
// which are handled by `align_to`.
Copy link
Contributor

Choose a reason for hiding this comment

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

That's only safe if the [u8] is correctly aligned (and has a size that holds an integer multiple of usizes as you write here AFAIU). Might be worth mentioning that align_to also takes care of aligning properly (does it?).

Copy link
Member

Choose a reason for hiding this comment

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

The more precise safety condition would be that [u8; size_of::<usize>] has the same validity invariants as usize, but I would not block on that in this PR.

@bors
Copy link
Contributor

bors commented Nov 25, 2020

⌛ Testing commit a506461 with merge 3f7ccb4...

@bors
Copy link
Contributor

bors commented Nov 25, 2020

☀️ Test successful - checks-actions
Approved by: kodrAus
Pushing 3f7ccb4 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 25, 2020
@bors bors merged commit 3f7ccb4 into rust-lang:master Nov 25, 2020
@rustbot rustbot added this to the 1.50.0 milestone Nov 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

@sdroege sdroege sdroege left review comments

Assignees

@KodrAus KodrAus

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.50.0
Development

Successfully merging this pull request may close these issues.