Conversions between CStr, OsStr, Path and boxes #39594

clarfonthey · 2017-02-06T20:25:16Z

This closes a bit of the inconsistencies between CStr, OsStr, Path, and str, allowing people to create boxed versions of DSTs other than str and [T].

Full list of additions:

Default for Box<str>, Box<CStr>, Box<OsStr>
CString::into_boxed_c_str (feature gated)
OsString::into_boxed_os_str (feature gated)
Path::into_boxed_path (feature gated)
From<&CStr> for Box<CStr>
From<&OsStr> for Box<OsStr>
From<&Path> for Box<Path>

This also includes adding the internal methods:

sys::*::os_str::Buf::into_box
sys::*::os_str::Slice::{into_box, empty_box}
sys_common::wtf8::Wtf8Buf::into_box
sys_common::wtf8::Wtf8::{into_box, empty_box}

rust-highfive · 2017-02-06T20:25:22Z

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

bjorn3 · 2017-02-07T15:03:29Z

src/libstd/ffi/c_str.rs

I think this should be unstable.

Trait implementations are inherently stable, so, that's not an option.

alexcrichton · 2017-02-07T20:49:10Z

I requested that this be split off #39438 (thanks @clarcharr!) because we don't have a lot of precedent for this in the standard library with other DSTs like Path, OsStr, etc. These seem reasonable to me, however, and I'd be fine adding all of them all at once as well.

clarfonthey · 2017-02-07T22:20:53Z

I will add this for OsStr soon, although I need to find out a proper way to reconcile how the platform-specific types should be laid out. Path will come easily after that.

clarfonthey · 2017-02-08T04:10:17Z

I've added versions for OsStr and Path, and it'd be nice if someone took a quick glance over the code to make sure everything looks right. Because AppVeyor doesn't automatically do PRs at the moment, there might be a bug in the Windows code that won't be caught until bors tries to merge it.

clarfonthey · 2017-02-08T04:12:17Z

@aturon / @alexcrichton, I'll open up a tracking issue for the into_boxed_* methods once I get a nod that those methods are desired. This will be a bit of an odd change because the From impls will be immediately stable but the into_* methods will not be. If it makes more sense to open an RFC for this I can do that.

alexcrichton · 2017-02-08T18:24:43Z

Nah yeah we can deal with the tracking issue just before merging, and these impls shouldn't require an RFC. Thanks for the updates!

clarfonthey · 2017-02-10T06:18:43Z

Currently waiting on #39438 which, even though it said it was merged before, was not actually merged.

clarfonthey · 2017-02-12T22:14:53Z

Waiting on CI, but I've implemented everything necessary for this and made a full list in the OP of what was implemented.

clarfonthey · 2017-02-13T05:56:23Z

Ping @aturon

aturon · 2017-02-13T16:31:15Z

@clarcharr Thanks! This looks good to me.

@rust-lang/libs: This PR involves some instantly-stable trait implementations. However, everything being implemented here is filling out our inter-conversion story to match other types in std, which should make things more consistent and predictable. I will likely r+ soon, so if you have objections or want time to consider further, please speak up!

aturon · 2017-02-13T16:37:24Z

@clarcharr One thing that makes me a bit uncomfortable about the implementation here is the pervasive use of mem::transmute. I wonder if we can at least factor out some common conversions into some free-standing functions that can be re-used? Each transmute is basically making an assertion about the relative representations, and it'd be good to consolidate those if we can.

BurntSushi · 2017-02-13T16:39:15Z

I think I'm fine with this.

In the implementation there are a lot of transmutes. Are they all necessary or is there anyway to get rid of some of them?

BurntSushi · 2017-02-13T16:39:52Z

@aturon Haha. Beat me to it. :P

alexcrichton · 2017-02-13T17:33:03Z

Looks good to me! I think we could annotate the input/output types in the transmutes, but otherwise I think they may unfortunately be required

clarfonthey · 2017-02-13T18:02:53Z

The main issue with the transmutes is that there's no other way of converting newtype DSTs. I decided to do one type at a time which is why there are so many, because each transmute adds one more layer of newtypes.

It's most complicated in the instance of Path on Windows, which is [u8] -> Wtf8 -> Slice -> OsStr -> Path.

I could just coerce an empty u8 slice to a path, but that makes it harder to understand the conversion that's actually happening.

aturon · 2017-02-13T22:43:28Z

Alright, given that the transmutes appear necessary, let's go ahead and land this!

@bor: r+

aturon · 2017-02-13T22:45:06Z

@bors: r+

bors · 2017-02-13T22:45:07Z

📌 Commit 9ed53cf has been approved by aturon

Conversions between CStr, OsStr, Path and boxes This closes a bit of the inconsistencies between `CStr`, `OsStr`, `Path`, and `str`, allowing people to create boxed versions of DSTs other than `str` and `[T]`. Full list of additions: * `Default` for `Box<str>`, `Box<CStr>`, `Box<OsStr>`, and `Box<Path>` (note: `Default` for `PathBuf` is already implemented) * `CString::into_boxed_c_str` (feature gated) * `OsString::into_boxed_os_str` (feature gated) * `Path::into_boxed_path` (feature gated) * `From<&CStr> for Box<CStr>` * `From<&OsStr> for Box<OsStr>` * `From<&Path> for Box<Path>` This also includes adding the internal methods: * `sys::*::os_str::Buf::into_box` * `sys::*::os_str::Slice::{into_box, empty_box}` * `sys_common::wtf8::Wtf8Buf::into_box` * `sys_common::wtf8::Wtf8::{into_box, empty_box}`

frewsxcv · 2017-02-14T04:34:59Z

@bors r-

This fails to compile on Windows: https://ci.appveyor.com/project/rust-lang/rust/build/1.0.1944/job/cu5i3oo7m5dq2hbg

clarfonthey · 2017-02-14T19:19:43Z

Should be working now.

alexcrichton · 2017-02-14T20:33:05Z

@bors: r=aturon

bors · 2017-02-14T20:33:06Z

📌 Commit 963843b has been approved by aturon

Conversions between CStr, OsStr, Path and boxes This closes a bit of the inconsistencies between `CStr`, `OsStr`, `Path`, and `str`, allowing people to create boxed versions of DSTs other than `str` and `[T]`. Full list of additions: * `Default` for `Box<str>`, `Box<CStr>`, `Box<OsStr>`, and `Box<Path>` (note: `Default` for `PathBuf` is already implemented) * `CString::into_boxed_c_str` (feature gated) * `OsString::into_boxed_os_str` (feature gated) * `Path::into_boxed_path` (feature gated) * `From<&CStr> for Box<CStr>` * `From<&OsStr> for Box<OsStr>` * `From<&Path> for Box<Path>` This also includes adding the internal methods: * `sys::*::os_str::Buf::into_box` * `sys::*::os_str::Slice::{into_box, empty_box}` * `sys_common::wtf8::Wtf8Buf::into_box` * `sys_common::wtf8::Wtf8::{into_box, empty_box}`

bors · 2017-02-15T10:22:38Z

⌛ Testing commit 963843b with merge e0044bd...

Conversions between CStr, OsStr, Path and boxes This closes a bit of the inconsistencies between `CStr`, `OsStr`, `Path`, and `str`, allowing people to create boxed versions of DSTs other than `str` and `[T]`. Full list of additions: * `Default` for `Box<str>`, `Box<CStr>`, `Box<OsStr>`, and `Box<Path>` (note: `Default` for `PathBuf` is already implemented) * `CString::into_boxed_c_str` (feature gated) * `OsString::into_boxed_os_str` (feature gated) * `Path::into_boxed_path` (feature gated) * `From<&CStr> for Box<CStr>` * `From<&OsStr> for Box<OsStr>` * `From<&Path> for Box<Path>` This also includes adding the internal methods: * `sys::*::os_str::Buf::into_box` * `sys::*::os_str::Slice::{into_box, empty_box}` * `sys_common::wtf8::Wtf8Buf::into_box` * `sys_common::wtf8::Wtf8::{into_box, empty_box}`

bors · 2017-02-15T13:01:29Z

☀️ Test successful - status-appveyor, status-travis
Approved by: aturon
Pushing e0044bd to master...

jonhoo · 2017-03-04T04:34:43Z

Out of curiosity, why is into_boxed_c_str behind a feature flag for now?

clarfonthey · 2017-03-04T05:19:36Z

@jonhoo, by protocol a tracking issue is opened for every new feature. So you'll have to wait until this goes stable for that to happen.

clarfonthey · 2017-03-04T05:20:20Z

I also realised that I never made a tracking issue for this, so, feel free to open a PR to add one in so that this can be nominated faster.

Leftovers from #39594; From<Box> impls These are a few more impls that follow the same reasoning as those from #39594. What's included: * `From<Box<str>> for String` * `From<Box<[T]>> for Vec<T>` * `From<Box<CStr>> for CString` * `From<Box<OsStr>> for OsString` * `From<Box<Path>> for PathBuf` * `Into<Box<str>> for String` * `Into<Box<[T]>> for Vec<T>` * `Into<Box<CStr>> for CString` * `Into<Box<OsStr>> for OsString` * `Into<Box<Path>> for PathBuf` * `<Box<CStr>>::into_c_string` * `<Box<OsStr>>::into_os_string` * `<Box<Path>>::into_path_buf` * Tracking issue for latter three methods + three from previous PR. Currently, the opposite direction isn't doable with `From` (only `Into`) because of the separation between `liballoc` and `libcollections`. I'm holding off on those for a later PR.

rust-highfive assigned aturon Feb 6, 2017

clarfonthey force-pushed the cstr_box branch from 32c194c to 92fb8fb Compare February 6, 2017 20:25

alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Feb 6, 2017

clarfonthey force-pushed the cstr_box branch from 92fb8fb to bf35a61 Compare February 6, 2017 23:54

bjorn3 requested changes Feb 7, 2017

View reviewed changes

clarfonthey force-pushed the cstr_box branch from 732b396 to 87d9b72 Compare February 8, 2017 04:08

clarfonthey changed the title ~~Conversions between CStr, CString, and Box<CStr>~~ Conversions between CStr, OsStr, Path and boxes Feb 8, 2017

clarfonthey force-pushed the cstr_box branch 3 times, most recently from a878b3d to a0bb95c Compare February 10, 2017 06:01

clarfonthey force-pushed the cstr_box branch 3 times, most recently from f4db2cb to 6dbd0f8 Compare February 12, 2017 22:09

clarfonthey force-pushed the cstr_box branch 3 times, most recently from 72f5273 to 9ed53cf Compare February 12, 2017 23:11

frewsxcv mentioned this pull request Feb 14, 2017

Rollup of 10 pull requests #39800

Closed

Conversions between CStr/OsStr/Path and boxes.

963843b

clarfonthey force-pushed the cstr_box branch from 9ed53cf to 963843b Compare February 14, 2017 19:19

frewsxcv mentioned this pull request Feb 15, 2017

Rollup of 6 pull requests #39833

Closed

bors merged commit 963843b into rust-lang:master Feb 15, 2017

clarfonthey deleted the cstr_box branch February 15, 2017 19:12

clarfonthey mentioned this pull request Feb 21, 2017

Leftovers from #39594; From<Box> impls #40009

Merged

alexcrichton mentioned this pull request Mar 3, 2017

No equivalent of into_boxed_str for CString/CStr #40248

Closed

clarfonthey mentioned this pull request Mar 9, 2017

Tracking issue for CString, OsString, PathBuf extra methods #40380

Closed

Conversions between CStr, OsStr, Path and boxes #39594

Conversions between CStr, OsStr, Path and boxes #39594

Uh oh!

Conversation

clarfonthey commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Feb 6, 2017

Uh oh!

bjorn3 Feb 7, 2017

Choose a reason for hiding this comment

Uh oh!

clarfonthey Feb 7, 2017

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Feb 7, 2017

Uh oh!

clarfonthey commented Feb 7, 2017

Uh oh!

clarfonthey commented Feb 8, 2017

Uh oh!

clarfonthey commented Feb 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexcrichton commented Feb 8, 2017

Uh oh!

clarfonthey commented Feb 10, 2017

Uh oh!

clarfonthey commented Feb 12, 2017

Uh oh!

clarfonthey commented Feb 13, 2017

Uh oh!

aturon commented Feb 13, 2017

Uh oh!

aturon commented Feb 13, 2017

Uh oh!

BurntSushi commented Feb 13, 2017

Uh oh!

BurntSushi commented Feb 13, 2017

Uh oh!

alexcrichton commented Feb 13, 2017

Uh oh!

clarfonthey commented Feb 13, 2017

Uh oh!

aturon commented Feb 13, 2017

Uh oh!

aturon commented Feb 13, 2017

Uh oh!

bors commented Feb 13, 2017

Uh oh!

frewsxcv commented Feb 14, 2017

Uh oh!

clarfonthey commented Feb 14, 2017

Uh oh!

alexcrichton commented Feb 14, 2017

Uh oh!

bors commented Feb 14, 2017

Uh oh!

bors commented Feb 15, 2017

Uh oh!

bors commented Feb 15, 2017

Uh oh!

jonhoo commented Mar 4, 2017

Uh oh!

clarfonthey commented Mar 4, 2017

Uh oh!

clarfonthey commented Mar 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

clarfonthey commented Feb 6, 2017 •

edited

Loading

clarfonthey commented Feb 8, 2017 •

edited

Loading