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

Documentation for CStr is not that of CString #84000

Closed
wants to merge 1 commit into from
Closed

Documentation for CStr is not that of CString #84000

wants to merge 1 commit into from

Conversation

eggyal
Copy link
Contributor

@eggyal eggyal commented Apr 8, 2021

Fixes #83999

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(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 8, 2021
jyn514
jyn514 reviewed Apr 8, 2021
View reviewed changes
@eggyal eggyal force-pushed the issue-83999 branch 3 times, most recently from 61aa43d to 72cac1c Compare April 8, 2021 17:10
m-ou-se
m-ou-se reviewed Apr 11, 2021
View reviewed changes
@m-ou-se m-ou-se 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 Apr 25, 2021
@bstrie
Copy link
Contributor

bstrie commented May 12, 2021

@eggyal , have you had time to address the most recent comments?

@eggyal
Copy link
Contributor Author

eggyal commented May 13, 2021

@bstrie @m-ou-se Apologies, this rather slipped off my radar... I'll get around to it later today.

@eggyal eggyal closed this May 14, 2021
@eggyal
Copy link
Contributor Author

eggyal commented May 14, 2021 •
edited
Loading

Erroneous push closed PR accidentally.

@rustbot label -S-waiting-on-author +S-waiting-on-review

@eggyal eggyal reopened this May 14, 2021
@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 14, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@eggyal eggyal force-pushed the issue-83999 branch 2 times, most recently from d16d8ca to 053e0c7 Compare May 14, 2021 10:17
@eggyal eggyal marked this pull request as draft May 14, 2021 10:19
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@eggyal
Copy link
Contributor Author

eggyal commented May 14, 2021

Aaagh, sorry, I'm making a right hash of git today.

@eggyal eggyal marked this pull request as ready for review May 14, 2021 10:25
m-ou-se
m-ou-se reviewed May 19, 2021
View reviewed changes
Copy link
Member

@m-ou-se m-ou-se left a comment

Choose a reason for hiding this comment

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

I don't think this change in its current form is an improvement. The original example shows the subtle mistake of using CString::new and .as_ptr() in the same expression, which is an easy mistake to make. The new example shows a more contrived snippet, which doesn't show as clearly how easy it is to make this mistake.

In addition, the changed documentation continues to explain a fix that doesn't just keep the right variable alive longer, but uses a completely different method of using a [u8; _] called bytes which the first example doesn't have, making it harder to focus on the lifetime problem itself.

@m-ou-se m-ou-se added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 19, 2021
@crlf0710
Copy link
Member

crlf0710 commented Jun 5, 2021

@eggyal Ping from triage, any updates on this?

@JohnCSimon JohnCSimon added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Jun 22, 2021
@JohnCSimon
Copy link
Member

@eggyal Ping from triage: I'm closing this as inactive because it hasn't moved for a month. Please feel free to reopen when you're ready to continue.

@JohnCSimon JohnCSimon closed this Jun 22, 2021
@eggyal eggyal deleted the issue-83999 branch June 28, 2022 06:11
This was referenced Jan 30, 2025
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m-ou-se m-ou-se m-ou-se left review comments

@gribozavr gribozavr gribozavr left review comments

@jyn514 jyn514 jyn514 left review comments

Assignees

@m-ou-se m-ou-se

Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Docs for CStr.as_ptr() are describing CString
10 participants
@eggyal @rust-highfive @bstrie @rust-log-analyzer @crlf0710 @JohnCSimon @m-ou-se @gribozavr @jyn514 @rustbot