Deprecate some unsafe functions in str::raw and remove OwnedStr trait

[aochagavia]

Removed the OwnedStr trait

This trait was only implemented by String. It provided the methods into_bytes and append, both of which are already implemented as normal methods of String (they were duplicated). This change improves the consistency of strings.

Its removal shouldn't break any code, except if somebody has implemented OwnedStr for his own types.

Deprecated some unsafe functions

• from_buf_len in favor of string::raw::from_buf_len
• from_c_str in favor of string::raw::from_buf
• from_utf8_owned in favor of string::raw::from_utf8
• from_byte in favor of string::raw::from_utf8 (make a new vector with a single byte and call the function)
• String::from_raw_parts in favor of string::raw::from_parts

[breaking-change]

[aochagavia]

cc @aturon

[SSheldon]

Now that this is in the same module as String, the transmute could be removed to just be String { vec: bytes }, yeah?

[aochagavia]

Good idea!
On Jul 19, 2014 8:37 PM, "Steven Sheldon" notifications@github.com wrote:

In src/libcollections/string.rs:

@@ -386,6 +400,14 @@ impl String {
}
}

• /// Converts a vector of bytes to a new String without checking if
• /// it contains valid UTF-8. This is unsafe because it assumes that
• /// the utf-8-ness of the vector has already been validated.
• #[inline]
• pub unsafe fn from_bytes(bytes: Vec) -> String {

•    mem::transmute(bytes)
  

Now that this is in the same module as String, the transmute could be
removed to just be String { vec: bytes }, yeah?

—
Reply to this email directly or view it on GitHub
https://github.com/rust-lang/rust/pull/15807/files#r15145056.

[aochagavia]

This is now ready to merge! :)

[alexcrichton]

I'm personally not a huge fan of CString::new(foo, false).as_str().unwrap().to_string() vs String::from_c_str(foo), but I agree that it is more consistent.

[aochagavia]

In #15859 I implement Show for CString. If that gets merged you could actually use CString::new(foo, false).to_string()

[aturon]

@aochagavia Could you spell out in more detail the motivation for deprecating/moving the functionality in raw? As a general rule, APIs shy away from exporting unsafe functions except in very explicit ways, and raw submodules are a common way to do this. (See for example the draft gudelines on opt-out of dynamic enforcement)

[aochagavia]

@aturon I don't pretend to deprecate all functions in raw, but only the ones returning a String. I have replaced them by methods of String, because it seems to me that it is the right place for them.

[aturon]

@aochagavia Right, I think I understand the general thinking here that constructors for a given type live directly on that type.

But for unsafe constructors/operations in particular, we have a widespread convention of placing them in a raw submodule instead, to make it very clear that they're meant for working directly with the underlying representation of the type. This is partly a way of discouraging their use except for the lowest-level code. I think we're generally happy with that convention.

[aochagavia]

String::from_utf8_unchecked looks much better to me than str::raw::from_utf8_owned, which looks as if it returned a &str.

Maybe an alternative is to create a raw module inside of string.

[aturon]

@aochagavia Ah, yes, I would definitely be in favor of providing these functions as string::raw.

[aochagavia]

Then I will make the necessary changes and post here when I am done. Thanks for reviewing!

[aturon]

@aochagavia My pleasure -- and thanks for continuing this work!

[aochagavia]

Should I also move the from_raw_parts unsafe constructor already present in String?

[aturon]

@aochagavia Yes, please move from_raw_parts as well.

[aochagavia]

I have moved the functions to string::raw. I renamed String::from_raw_parts to string::raw::from_parts because I thought it was better than having string::raw::from_raw_parts (two times raw).

Now we have to wait for Travis...

[alexcrichton]

r? @aturon

Also, for comparison, the std::str::raw module seems like it should closely mirror the std::string::raw module, and the std::str modules contains c_str_to_static_slice which can be construed as analgous to from_c_str.

[aturon]

I agree that we should align std::str::raw.

@aochagavia Looks like this is passing Travis now, and the code looks good to me, but before I sign off, I wanted to ask how you'd feel about a string::raw::from_buf, as a replacement for from_c_str (and analogous to string::raw::from_buf_len? If we're offering from_buf_len, it seems to me we could also give the convenience of from_buf. That would resolve the ergonomic regression here, and I think still give a consistent API.

Thoughts?

[aochagavia]

I think it is a good idea! Let's do it.

[aochagavia]

I have added the from_buf function.

Additionaly, I have reordered the arguments of from_parts to match the order of from_buf and from_buf_len: first the pointer, then the lenght, then the capacity.

[aturon]

@aochagavia Thanks! One more request: there were a few places in the patch that were previously using from_c_str and are now going through CString somewhat verbosely. Can those go through from_buf instead?

[aturon]

This should now test from_buf.

[aturon]

(and move to string to do so)

[aochagavia]

done!

[aochagavia]

I have already catched those 😉

EDIT: it seems that I didn't catch all of them!

[aturon]

@aochagavia Note Travis error.

[aochagavia]

This should be ok now