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

fix ptr_rotate comments #52502

Merged
merged 1 commit into from
Jul 21, 2018
Merged

fix ptr_rotate comments #52502

merged 1 commit into from
Jul 21, 2018

Conversation

RalfJung
Copy link
Member

rotate::ptr_rotate has a comment saying

/// # Safety
///
/// The specified range must be valid for reading and writing.
/// The type `T` must have non-zero size.

So we better make sure we don't call it on ZST...

Cc @scottmcm (author of #41670)

@rust-highfive
Copy link
Collaborator

r? @bluss

(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 Jul 18, 2018
@scottmcm
Copy link
Member

Thanks for noticing this, Ralf! Now to see if I can remember what I was thinking over a year ago...

My first instinct is that that line's there from a previous iteration and I just forgot to remove it when I make ZSTs works, since there's explicit code to handle ZSTs properly in rotate.rs:

fn cap() -> usize {
if mem::size_of::<T>() == 0 {
usize::max_value()
} else {
mem::size_of::<Self>() / mem::size_of::<T>()
}
}

Looking deeper into ptr_rotate, it's going to immediately break out of the first loop because the amount needing rotating always fits in the usize::MAX buffer capacity. So then it does some offsets and ptr::copys that of course do nothing because they're on a ZST pointer. And done.

So maybe the answer is just to remove the incorrect line from the comment. LLVM does seem to already completely compile out all the logic for ZSTs, leaving only the bounds check.

@RalfJung
Copy link
Member Author

Looking deeper into ptr_rotate, it's going to immediately break out of the first loop because the amount needing rotating always fits in the usize::MAX buffer capacity.

Ack.

So then it does some offsets and ptr::copys that of course do nothing because they're on a ZST pointer. And done.

Ah, indeed copy is per-element as well.

@RalfJung
Copy link
Member Author

I replaced the PR by a commit that just changes the comment.

Copy link
Member

@scottmcm scottmcm left a comment

Choose a reason for hiding this comment

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

Looks good; I like the new comment about cap-for-ZST.

@RalfJung
Copy link
Member Author

So, does that mean r+? :D

@scottmcm
Copy link
Member

If I had permissions to do that it would 🙃

@kennytm
Copy link
Member

kennytm commented Jul 20, 2018

@bors r=scottmcm rollup

@bors
Copy link
Contributor

bors commented Jul 20, 2018

📌 Commit 16c0572 has been approved by scottmcm

@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 Jul 20, 2018
@kennytm
Copy link
Member

kennytm commented Jul 20, 2018

(BTW @RalfJung maybe change the PR title 🙂)

@RalfJung
Copy link
Member Author

If I had permissions to do that it would upside_down_face

oops sorry^^

(BTW @RalfJung maybe change the PR title )

Will do :D

@RalfJung RalfJung changed the title fix unsafety: don't call ptr_rotate for ZST fix ptr_rotate comments Jul 20, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Jul 20, 2018
fix unsafety: don't call ptr_rotate for ZST

`rotate::ptr_rotate` has a comment saying
```
/// # Safety
///
/// The specified range must be valid for reading and writing.
/// The type `T` must have non-zero size.
```
So we better make sure we don't call it on ZST...

Cc @scottmcm (author of rust-lang#41670)
bors added a commit that referenced this pull request Jul 20, 2018
Rollup of 7 pull requests

Successful merges:

 - #52502 (fix unsafety: don't call ptr_rotate for ZST)
 - #52505 (rustc: Remove a workaround in ThinLTO fixed upstream)
 - #52526 (Enable run-pass/sepcomp-lib-lto.rs on Android)
 - #52527 (Remove duplicate E0396 tests)
 - #52539 (rustc: Fix two custom attributes with custom derive)
 - #52540 (Fix docker/run.sh script when run locally)
 - #52573 (Cleanups)

Failed merges:

r? @ghost
@bors bors merged commit 16c0572 into rust-lang:master Jul 21, 2018
@RalfJung RalfJung deleted the rotate branch July 23, 2018 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants