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

extend from_raw_parts docs for slices and strs to mention alignment requirement #51134

Merged
merged 2 commits into from May 29, 2018

Conversation

RalfJung
Copy link
Member

The documentation for str::from_raw_parts_mut seems to not be visible because that method is private, bit I figured it could still be fixed. I also removed the reference to the no-longer-existing str::from_raw_parts while I was at it.

Alternatively, should I remove str::from_raw_parts_mut completely? it is only used in str::split_at_mut, where it might as well be inlined.

@hanna-kruppe
Copy link
Contributor

The changes to str::from_raw_parts_mut are kind of pointless since any u8 pointer is aligned. One more reason to kill it (inline it to its sole use site), I'd say.

@RalfJung
Copy link
Member Author

@rust-highfive seems to be on vacation?

str::from_raw_parts has been removed long ago because it can be obtained via
str::from_utf8_unchecked and slice::from_raw_parts.  The same goes for
str::from_raw_parts_mut.
@RalfJung
Copy link
Member Author

The changes to str::from_raw_parts_mut are kind of pointless since any u8 pointer is aligned.

Oh, good point.

One more reason to kill it (inline it to its sole use site), I'd say.

Done.

@RalfJung
Copy link
Member Author

r? @SimonSapin

(@rust-highfive somehow missed this PR so I picked someone from the libs team...)

@varkor varkor added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 29, 2018
@SimonSapin
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 29, 2018

📌 Commit b30aaf2 has been approved by SimonSapin

@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 May 29, 2018
@bors
Copy link
Contributor

bors commented May 29, 2018

⌛ Testing commit b30aaf2 with merge 889d8dc...

bors added a commit that referenced this pull request May 29, 2018
extend from_raw_parts docs for slices and strs to mention alignment requirement

The documentation for `str::from_raw_parts_mut` seems to not be visible because that method is private, bit I figured it could still be fixed. I also removed the reference to the no-longer-existing `str::from_raw_parts` while I was at it.

Alternatively, should I remove `str::from_raw_parts_mut` completely? it is only used in `str::split_at_mut`, where it might as well be inlined.
@bors
Copy link
Contributor

bors commented May 29, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: SimonSapin
Pushing 889d8dc to master...

@bors bors merged commit b30aaf2 into rust-lang:master May 29, 2018
@hsivonen
Copy link
Member

hsivonen commented Jun 4, 2018

It's good that this change documented the new alignment requirement. Thanks. It's unfortunate that it also removed the explanation why the pointer has to be non-null. (People are more likely to obey the rules when they understand the rationale.)

@RalfJung
Copy link
Member Author

RalfJung commented Jun 4, 2018

The problem is that enum layout optimizations currently only exploit non-nullness, not alignment -- and it seemed weird to me to provide explanation for some but not all of the requirements.

We could mention the arcane LLVM flags we are setting (both for nunnull and aligned), but I don't feel that would explain anything. The thing is, even if the layout optimizations were turned off, it'd still be UB due to these flags.

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.

None yet

6 participants