DSTify ToCStr #18457
DSTify ToCStr #18457
Conversation
|
|
| unsafe fn with_c_str_unchecked<T>(&self, f: |*const libc::c_char| -> T) -> T { | ||
| (**self).with_c_str_unchecked(f) | ||
| } | ||
| } |
There was a problem hiding this comment.
Would this be better expressed by impl<'a, T: ToCStr> ToCStr for &'a T ?
There was a problem hiding this comment.
Actually this is only necessary because of the use case of cmd.args(&["foo", "bar"]). If we move to a iterator centric API this could be removed (maybe, I haven't give it too much thought).
|
While I think it makes sense to impl Perhaps |
|
The // path has type &Path
// Take ToCStr by value
Command::new(path.clone()); // allocates for Path (just to satisfy the by-value need)
// and allocates again for CString
// vs
// Take ToCStr by reference
Command::new(path); // only one allocation for the CStringAlthough I didn't find any unnecessary clone in this PR, I did find a few unnecessary clones in my |
|
Note that if we |
That sounds good to me. (blanket impl for @aturon What do you think? |
|
I'm comfortable with that. Given that we're already taking a generic parameter for easy overloading, may as well let you pass owned data when convenient. I would suggest making the |
|
Updated to use @alexcrichton proposal and squashed. @aturon re-r? |
Methods that used to take `ToCStr` implementors by value, now take them by reference. In particular, this breaks some uses of `Command`:
``` rust
Command::new("foo"); // Still works
Command::new(path) -> Command::new(&path)
cmd.arg(string) -> cmd.arg(&string) or cmd.arg(string.as_slice())
```
[breaking-change]
---
It may be sensible to remove `impl ToCstr for String` since:
- We're getting `impl Deref<str> for String`, so `string.to_cstr()` would still work
- `Command` methods would still be able to use `cmd.arg(string[..])` instead of `cmd.arg(&string)`.
But, I'm leaving that up to the library stabilization process.
r? @aturon
cc #16918
- The `BytesContainer::container_into_owned_bytes` method has been removed
- Methods that used to take `BytesContainer` implementors by value, now take them by reference. In particular, this breaks some uses of Path:
``` rust
Path::new("foo") // Still works
path.join(another_path) -> path.join(&another_path)
```
[breaking-change]
---
Re: `container_into_owned_bytes`, I've removed it because
- Nothing in the whole repository uses it
- Takes `self` by value, which is incompatible with unsized types (`str`)
The alternative to removing this method is to split `BytesContainer` into `BytesContainer for Sized?` and `SizedBytesContainer: BytesContainer + Sized`, where the second trait only contains the `container_into_owned_bytes` method. I tried this alternative [in another branch](https://github.com/japaric/rust/commits/bytes) and it works, but it seemed better not to create a new trait for an unused method.
Re: Breakage of `Path` methods
We could use the idea that @alexcrichton proposed in #18457 (add blanket `impl BytesContainer for &T where T: BytesContainer` + keep taking `T: BytesContainer` by value in `Path` methods) to avoid breaking any code.
r? @aturon
cc #16918
Methods that used to take
ToCStrimplementors by value, now take them by reference. In particular, this breaks some uses ofCommand:[breaking-change]
It may be sensible to remove
impl ToCstr for Stringsince:impl Deref<str> for String, sostring.to_cstr()would still workCommandmethods would still be able to usecmd.arg(string[..])instead ofcmd.arg(&string).But, I'm leaving that up to the library stabilization process.
r? @aturon
cc #16918