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

Improve the safety docs for CStr #95948

Merged
merged 1 commit into from
Jun 7, 2022
Merged

Improve the safety docs for CStr #95948

merged 1 commit into from
Jun 7, 2022

Conversation

Noratrieb
Copy link
Member

@Noratrieb Noratrieb commented Apr 11, 2022 •
edited
Loading

Namely, the two functions from_ptr and from_bytes_with_nul_unchecked.
Before, these functions didn't state the requirements clearly enough,
and I was not immediately able to find them like for other functions.

This doesn't change the content of the docs, but simply rewords them for
clarity.

note: I'm not entirely sure about the 'ptr must be valid for reads of u8.', there might be room for improvement for this (and maybe for the other docs as well 😄)

@rust-highfive
Copy link
Collaborator

Thank you for submitting a new PR for the library teams! If this PR contains a stabilization of a library feature that has not already completed FCP in its tracking issue, introduces new or changes existing unstable library APIs, or changes our public documentation in ways that create new stability guarantees then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(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 Apr 11, 2022
@bors
Copy link
Contributor

bors commented Apr 15, 2022

☔ The latest upstream changes (presumably #94079) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 23, 2022
@Noratrieb
Copy link
Member Author

Noratrieb commented Apr 23, 2022 •
edited
Loading

Fixed the merge conflict related to CStr being moved to core, should be good to go

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2022
@JohnTitor
Copy link
Member

r? @RalfJung maybe? I assume you're familiar with that area. If not feel free to re-assign!

@Dylan-DPC Dylan-DPC 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 May 23, 2022
@Noratrieb
Copy link
Member Author

@rustbot ready

@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 May 23, 2022
@Noratrieb
Copy link
Member Author

Thank you for the review, I've implemented the suggestions!

RalfJung
RalfJung reviewed May 24, 2022
View reviewed changes
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

Thanks for the edits!

However, this still reads quite different from https://doc.rust-lang.org/nightly/std/slice/fn.from_raw_parts.html, doesn't it?

library/core/src/ffi/c_str.rs Outdated Show resolved Hide resolved
library/core/src/ffi/c_str.rs Outdated Show resolved Hide resolved
library/core/src/ffi/c_str.rs Outdated Show resolved Hide resolved
library/core/src/ffi/c_str.rs Outdated Show resolved Hide resolved
@Noratrieb
Copy link
Member Author

I applied your suggestions and also added the Caveat section from slice::from_raw_parts, since I though this would be useful here too.

RalfJung
RalfJung reviewed May 28, 2022
View reviewed changes
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

I like it, thanks. :) Just some last nits, then we are done.

library/core/src/ffi/c_str.rs Outdated Show resolved Hide resolved
library/core/src/ffi/c_str.rs Outdated Show resolved Hide resolved
library/core/src/ffi/c_str.rs Outdated Show resolved Hide resolved
@Noratrieb
Copy link
Member Author

@RalfJung I think it should be ready now :)

@RalfJung
Copy link
Member

RalfJung commented Jun 7, 2022

Thanks for the ping; GH doesn't always send notifications for force pushes so I did not realize this is ready for review again.

@RalfJung
Copy link
Member

RalfJung commented Jun 7, 2022

However, the 3 nits that I posted are all still open I think?

@Noratrieb
Copy link
Member Author

Huh, that's weird, I remember fixing those. Anyways, I'm quickly gonna fix them.

@Noratrieb
Improve the safety docs for CStr
0dda42b 
Namely, the two functions `from_ptr` and `from_bytes_with_nul_unchecked`.
Before, this functions didn't state the requirements clearly enough,
and I was not immediately able to find them like for other functions.

This doesn't change the content of the docs, but simply rewords them for
clarity.
@Noratrieb
Copy link
Member Author

And fixed it.

@RalfJung
Copy link
Member

RalfJung commented Jun 7, 2022

Thanks. :)
@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 7, 2022

📌 Commit 0dda42b has been approved by RalfJung

@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 Jun 7, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 7, 2022
@Dylan-DPC
Rollup merge of rust-lang#95948 - Nilstrieb:improve-cstr-safety-docs,…
e404c99 
… r=RalfJung

Improve the safety docs for `CStr`

Namely, the two functions `from_ptr` and `from_bytes_with_nul_unchecked`.
Before, these functions didn't state the requirements clearly enough,
and I was not immediately able to find them like for other functions.

This doesn't change the content of the docs, but simply rewords them for
clarity.

note: I'm not entirely sure about the '`ptr` must be valid for reads of `u8`.', there might be room for improvement for this (and maybe for the other docs as well 😄)
This was referenced Jun 7, 2022
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 7, 2022
@bors
Auto merge of rust-lang#97835 - Dylan-DPC:rollup-0ae3pwp, r=Dylan-DPC
5435ed6 
Rollup of 5 pull requests

Successful merges:

 - rust-lang#95948 (Improve the safety docs for `CStr`)
 - rust-lang#97325 (Fix precise field capture of univariant enums)
 - rust-lang#97817 (:arrow_up: rust-analyzer)
 - rust-lang#97821 (Remove confusing sentence from `Mutex` docs)
 - rust-lang#97826 (Add more information for rustdoc-gui tests)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a9c4a7e into rust-lang:master Jun 7, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 7, 2022
@Noratrieb Noratrieb deleted the improve-cstr-safety-docs branch June 8, 2022 05:54
@SoniEx2 SoniEx2 mentioned this pull request Aug 13, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

@RalfJung RalfJung

Labels
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.
Projects
None yet
Milestone
1.63.0
Development

Successfully merging this pull request may close these issues.
9 participants
@Noratrieb @rust-highfive @bors @JohnTitor @RalfJung @joshtriplett @JohnCSimon @rustbot @Dylan-DPC