Skip to content

Implement fixed-size-buffer constructors for CString #46795

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

Closed
wants to merge 2 commits into from

Conversation

Diggsey
Copy link
Contributor

@Diggsey Diggsey commented Dec 17, 2017

Fixes #20475

I added three variants. Not sure about naming/whether we necessarily need all three.

impl CString {
    // Copy from raw C-style buffer into new CString
    unsafe fn of_fixed_size_raw(ptr: *const c_char, maxlen: usize) -> CString;

    // Copy from byte slice into new CString
    fn of_fixed_size(slice: &[u8]) -> CString;

    // Convert owned byte buffer into CString
    fn from_fixed_size<T: Into<Vec<u8>>>(vec: T) -> CString;
}

For comparison, the existing constructors are:

impl CString {
    // Input must not contain \0
    fn new<T: Into<Vec<u8>>>(t: T) -> Result<CString, NulError>;
    unsafe fn from_vec_unchecked(v: Vec<u8>) -> CString;

    // Must have come from CString::into_raw
    unsafe fn from_raw(ptr: *mut c_char) -> CString;
}

r? @dtolnay

@dtolnay dtolnay added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 18, 2017
@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 18, 2017
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

First impression: this seems a bit like a new string representation, next to str and CStr and OsStr. Is it possible to expose this in a crate as something like a BoundedCStr type with its own suite of constructors, and conversion to CString? What would the API look like and would it be able to express the same use cases efficiently as the API here?

I don't think we have used the fn of_ naming convention in the standard library yet. Are the two of_ constructors doing something novel that does not match any existing naming convention?

(Have not reviewed the implementation yet.)

@Diggsey
Copy link
Contributor Author

Diggsey commented Dec 27, 2017

First impression: this seems a bit like a new string representation

Yes, I'm starting to think this is the case

What would the API look like and would it be able to express the same use cases efficiently as the API here?

I will take a stab at designing this.

I don't think we have used the fn of_ naming convention in the standard library yet. Are the two of_ constructors doing something novel that does not match any existing naming convention?

Typically the constructors do not copy the data - new/from/etc. variants take ownership of the underlying storage. The pattern for constructing an owned type typically looks like: Borrowed::from(...).to_owned() - ultimately the lack of a convention stems from the same issue you pointed out above of this really being a new string representation.

@Diggsey
Copy link
Contributor Author

Diggsey commented Dec 30, 2017

@dtolnay https://docs.rs/c_fixed_string/0.1.0/c_fixed_string/

@dtolnay
Copy link
Member

dtolnay commented Dec 30, 2017

Amazing work @Diggsey! I think those types will be more convenient to use than the constructors in this PR because they do a better job capturing how this representation is different from CString. What do you think?

I guess the next thing to decide is whether this c-fixed-string representation is common enough that it makes sense pursuing these in the standard library, or whether we can direct people to a crate instead.

@Diggsey
Copy link
Contributor Author

Diggsey commented Dec 30, 2017

Particularly given the amount of API surface that's needed to support this, I think it will have to exist as a crate until it's seen a good amount of usage. Let's close this PR and resolve the original issue with a link to this crate.

@Diggsey Diggsey closed this Dec 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

CString should have a "within max length" constructor
3 participants