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

add CString::from_vec_until_nul #95160

Closed
wants to merge 0 commits into from

Conversation

ericseppanen
Copy link
Contributor

@ericseppanen ericseppanen commented Mar 21, 2022

This adds a fn that converts a Vec into a CString, without requiring the nul byte to be at the end of the Vec. It is intended for the case when an FFI function call wrote a string of unknown length, which from_vec_with_nul doesn't handle easily.

For example,

let mut buffer = vec![0u8; 32];
unsafe {
    some_c_function(buffer.as_mut_ptr(), buffer.len());
}
let result = CString::from_vec_until_nul(buffer).unwrap();

This is a companion to CStr::from_bytes_until_nul (#94984) , and shares the feature gate: cstr_from_bytes_until_nul (tracking issue #95027).

Unresolved questions:

I couldn't decide whether this fn should use its own unique error type, or whether it should share the error type with from_vec_with_nul. In this version I reused the existing error type, but I'm happy to change if that's the better choice.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 Mar 21, 2022
@ericseppanen
Copy link
Contributor Author

The biggest downside to sharing the error type is that the names obviously don't match:

pub fn from_vec_until_nul(mut v: Vec<u8>) -> Result<Self, FromVecWithNulError>

FromVecWithNulError also contains a FromBytesWithNulErrorKind enum which is not useful in this impl (because there's only one way this fn can fail.

I also find it odd that FromVecWithNulError returns the Vec that was input, but that member is private. I'm not sure why that choice was made originally-- maybe @poliorcetics would know?

@rust-log-analyzer

This comment has been minimized.

@ericseppanen
Copy link
Contributor Author

(fixed doc-test typo)

@poliorcetics
Copy link
Contributor

I also find it odd that FromVecWithNulError returns the Vec that was input, but that member is private. I'm not sure why that choice was made originally-- maybe @poliorcetics would know?

To allow extensibility of the internals, as unlikely as it is. There are functions on the type to access it IIRC

@petrochenkov
Copy link
Contributor

I'm going to block this on #94079 (which is almost ready), because all such changes have to be reapplied manually when rebasing #94079.

@petrochenkov petrochenkov added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. 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 Mar 21, 2022
@ghost
Copy link

ghost commented Mar 21, 2022

I think the example on this issue is supposed to consume the buffer:
CString::from_vec_until_nul(&buffer) -> CString::from_vec_until_nul(buffer)

(sorry for the minor nit, the & just jumped out to me as odd)

@ericseppanen
Copy link
Contributor Author

I think the example on this issue is supposed to consume the buffer: CString::from_vec_until_nul(&buffer) -> CString::from_vec_until_nul(buffer)

Fixed, thanks.

@ericseppanen
Copy link
Contributor Author

I also find it odd that FromVecWithNulError returns the Vec that was input, but that member is private. I'm not sure why that choice was made originally-- maybe @poliorcetics would know?

To allow extensibility of the internals, as unlikely as it is. There are functions on the type to access it IIRC

Ah, I missed that FromVecWithNulError has as_bytes and into_bytes methods. Thanks for the help!

@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.

@ericseppanen
Copy link
Contributor Author

Apologies for accidentally closing this; I don't seem to be able to re-open it so I opened a new PR #96186 with the same content.

@jyn514
Copy link
Member

jyn514 commented Apr 20, 2022

@ericseppanen you need to force push back to d4c9367 to be able to reopen the PR. (I think GitHub does this to avoid having two simultaneous PRs open from the same branch.)

@ericseppanen
Copy link
Contributor Author

@ericseppanen you need to force push back to d4c9367 to be able to reopen the PR. (I think GitHub does this to avoid having two simultaneous PRs open from the same branch.)

I tried that but it didn't seem to do anything.

@jyn514
Copy link
Member

jyn514 commented Apr 20, 2022

@ericseppanen
Copy link
Contributor Author

@jyn514 I did force push the branch to many different hashes, including the one you mentioned. Github stopped updating the "force-pushed from X to Y" message after the first one; I don't know why. After trying it a bunch of times I gave up and opened a new PR. I have updated the tracking issue to point to the new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants