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

Implemented FromStr for CString and TryFrom<CString> for String #130608

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

YohDeadfall
Copy link
Contributor

The motivation of this change is making it possible to use CString in generic methods with FromStr and TryInto<String> trait bounds. The same traits are already implemented for OsString which is an FFI type too.

@rustbot
Copy link
Collaborator

rustbot commented Sep 20, 2024

r? @workingjubilee

rustbot has assigned @workingjubilee.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 20, 2024
@jieyouxu jieyouxu added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 20, 2024
@workingjubilee workingjubilee added I-libs-api-nominated Nominated for discussion during a libs-api team meeting. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 20, 2024
@workingjubilee
Copy link
Member

This is a request to stabilize a trait, and as such needs a FCP from libs-api.

@YohDeadfall
Copy link
Contributor Author

Is anything required from my side now?

@workingjubilee
Copy link
Member

No. You could choose to provide additional explanatory justification for why this should be accepted, if you wish, but it's not required.

@YohDeadfall
Copy link
Contributor Author

Then there's nothing to add, actually. It's all about usability of the CString type in generics and orphan rules preventing from having these traits on the type. The issue can be worked around by making another trait for string types, but it's better to fix the issue in the first place since OsString is already doing that right.

Comment on lines +821 to +827
#[inline]
fn from_str(s: &str) -> Result<Self, Self::Err> {
Self::new(s)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be good to explicitly document that this does not require a terminating NUL in the input, maybe something like

Suggested change
#[inline]
fn from_str(s: &str) -> Result<Self, Self::Err> {
Self::new(s)
}
/// Creates a new C-compatible string from a `&str`.
///
/// The trailing 0 byte will be appended by this function. If the provided data
/// contains any 0 bytes in it, `Err(NulError)` will be returned.
#[inline]
fn from_str(s: &str) -> Result<Self, Self::Err> {
Self::new(s)
}

or maybe just link to the docs for CString::new.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a short description with a link to the corresponding function, and also did the same for the backward conversion.

@joshtriplett
Copy link
Member

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Sep 24, 2024

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 24, 2024
Copy link
Member

@joshtriplett joshtriplett left a comment

Choose a reason for hiding this comment

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

Needs to use TryFrom not TryInto.

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Oct 1, 2024
@rfcbot
Copy link

rfcbot commented Oct 1, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@Amanieu Amanieu removed needs-fcp This change is insta-stable, so needs a completed FCP to proceed. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. labels Oct 1, 2024
@slanterns
Copy link
Contributor

A squash is favorable.

@YohDeadfall YohDeadfall changed the title Implemented FromStr/TryInto<String> for CString Implemented FromStr for CString and TryFrom<CString> for String` Oct 1, 2024
@YohDeadfall YohDeadfall changed the title Implemented FromStr for CString and TryFrom<CString> for String` Implemented FromStr for CString and TryFrom<CString> for String Oct 1, 2024
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Oct 11, 2024
@rfcbot
Copy link

rfcbot commented Oct 11, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@workingjubilee
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 14, 2024

📌 Commit 3025513 has been approved by workingjubilee

It is now in the queue for this repository.

@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 14, 2024
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Oct 15, 2024
…workingjubilee

Implemented `FromStr` for `CString` and `TryFrom<CString>` for `String`

The motivation of this change is making it possible to use `CString` in generic methods with `FromStr` and `TryInto<String>` trait bounds. The same traits are already implemented for `OsString` which is an FFI type too.
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 15, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#130608 (Implemented `FromStr` for `CString` and `TryFrom<CString>` for `String`)
 - rust-lang#130635 (Add `&pin (mut|const) T` type position sugar)
 - rust-lang#130747 (improve error messages for `C-cmse-nonsecure-entry` functions)
 - rust-lang#131137 (Add 1.82 release notes)
 - rust-lang#131328 (Remove unnecessary sorts in `rustc_hir_analysis`)
 - rust-lang#131496 (Stabilise `const_make_ascii`.)
 - rust-lang#131706 (Fix two const-hacks)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3a00d35 into rust-lang:master Oct 15, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Oct 15, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 15, 2024
Rollup merge of rust-lang#130608 - YohDeadfall:cstr-from-into-str, r=workingjubilee

Implemented `FromStr` for `CString` and `TryFrom<CString>` for `String`

The motivation of this change is making it possible to use `CString` in generic methods with `FromStr` and `TryInto<String>` trait bounds. The same traits are already implemented for `OsString` which is an FFI type too.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. to-announce Announce this issue on triage meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants