Split CString::new into CString::new and CString::new_owned

[canndrew]

This is a fix for #18117

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

[rust-highfive]

Warning

• These commits modify unsafe code. Please review it carefully!

[nodakai]

s/i8/libc::c_char/

[nodakai]

The pair of names CString::new() and CString::new_owned() gives an impression that a CString doesn't usually "own" the underlying memory chunk and the "owned" variant is something special.

Note also that the module-wide document goes as follows:

A C string is managed through the CString type defined in this module. It "owns" the internal buffer of characters and will automatically deallocate the buffer when the string is dropped.

[canndrew]

s/i8/libc::c_char/

Good catch.

The pair of names CString::new() and CString::new_owned() gives an impression that a CString doesn't usually "own" the underlying memory chunk and the "owned" variant is something special.

That is the case though. The owned variant deallocates the buffer when it drops. I'll fix the documentation as well.

[huonw]

A C string is managed through the CString type defined in this module. It "owns" the internal buffer of characters and will automatically deallocate the buffer when the string is dropped.

@nodakai these docs are not very relevant, since they're wrong even now. new has signature:

unsafe fn new(buf: *const i8, owns_buffer: bool) -> CString

Passing false to owns_buffer will not deallocate the buffer.

[nodakai]

@canndrew @nikomatsakis What is the status of this PR?

[nikomatsakis]

@nodakai sorry for the delay, I've been at a conference and not keeping up with github. I'm going through PRs assigned to me now.

[nikomatsakis]

So, in general this looks to be high-quality work, but I know that there are existing efforts to revamp these APIs and I'm not sure how this PR fits in. I'm ccing @aturon who can provide more feedback on that.

I want to say thanks for the PR and also to apologize for the delay in reviewing this.

[aturon]

@canndrew Thanks for doing this work!

This seems like a reasonable change to the API, but I'm a bit worried that we actually need a more substantial overhaul: see #16772 which recommends breaking into two separate types.

More generally, I've been planning to revamp the CString and CVec interfaces overall, since they are quite old and we can now do better.

Because of the potential for these more drastic changes, I'm a little hesitant to land this smaller API change because of the extra churn it would cause.

Would you be interested in being involved with a larger revamp effort?

[aturon]

@canndrew To be clear, I'm not necessarily opposed to landing this incremental improvement now, I just want to think a bit about how it fits into broader plans.

[mzabaluev]

FYI, here's my crack at improving CString, taken outside of the std crate:
https://github.com/mzabaluev/rust-c-compat/blob/master/src/c_str.rs
Unfortunately, I did it unaware of this pull request. Anyway, I find new_unowned a better choice of name for the unowned constructor. I've also added destructor closures similar to CVec, with CString::new_libc as a common special case.

[canndrew]

@aturon Sure I'd be interested in being involved in a larger revamp effort. I actually think CString would be better off split into CString and CStringMut or something like that and moved into liblibc but I was hesitant to make such a significant change. It looks like @mzabaluev has already done most this work though.

[canndrew]

Closing in favor of @mzabaluev's RFC.