Skip to content

Conversation

clarfonthey
Copy link
Contributor

@clarfonthey clarfonthey commented Feb 21, 2017

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
Copy link
Contributor

r? @brson

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

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

Discussed during libs triage today the conclusion was that this looks good to go, but could the Default impl be removed?

@clarfonthey
Copy link
Contributor Author

@alexcrichton It can, although both PathBuf and Box<Path> impl Default now. I'm fine with moving it to another PR though!

@alexcrichton
Copy link
Member

Yes let's move to a separate PR

@clarfonthey
Copy link
Contributor Author

I separated out the Default impl and am working on making the WIP stuff work.

@clarfonthey clarfonthey force-pushed the box_to_buf branch 3 times, most recently from 54f1a9c to 416e25f Compare March 9, 2017 03:11
@clarfonthey clarfonthey force-pushed the box_to_buf branch 2 times, most recently from 92f4ac6 to 9a6fc62 Compare March 9, 2017 03:19
@clarfonthey
Copy link
Contributor Author

And I've finished the impls! Should work properly; waiting on Travis.

@clarfonthey
Copy link
Contributor Author

Apparently, Travis is failing because of a type mismatch that doesn't seem to exist? I'm going to test it out locally and see if I can fix it.

@clarfonthey
Copy link
Contributor Author

So, this seems pretty broken:

error[E0308]: mismatched types
    --> src/libcollections/string.rs:1980:9
     |
1980 |         s.into_string()
     |         ^^^^^^^^^^^^^^^ expected struct `string::String`, found struct `std::string::String`
     |
     = note: expected type `string::String`
                found type `std::string::String`

error[E0308]: mismatched types
    --> src/libcollections/vec.rs:1903:9
     |
1903 |         s.into_vec()
     |         ^^^^^^^^^^^^ expected struct `vec::Vec`, found struct `std::vec::Vec`
     |
     = note: expected type `vec::Vec<T>`
                found type `std::vec::Vec<T>`
     = help: here are some functions which might fulfill your needs:
             - .remove(...)
             - .swap_remove(...)

In what circumstances aren't those the same type? Another note: the implementations for those types do use the types they're asking for, which makes no sense to me.

@clarfonthey
Copy link
Contributor Author

Got some help on IRC from @thepowersgang (I think); apparently, this is causing the error:

#[cfg(test)]
#[macro_use]
extern crate std;

So right now I disabled the Box -> String and Box -> Vec conversions on tests. Shouldn't be too bad, considering how they just call an already-tested function.

@clarfonthey clarfonthey force-pushed the box_to_buf branch 2 times, most recently from e092016 to 2b8b7e3 Compare March 10, 2017 02:39
@clarfonthey
Copy link
Contributor Author

clarfonthey commented Mar 10, 2017

This is ready to merge now, @alexcrichton.

@alexcrichton
Copy link
Member

Looks like there may be travis errors?

@alexcrichton
Copy link
Member

This seems to have also grown impls since last we looked? The Into impls are a little unfortunate (but seem necessitated by coherence).

I'd be uncomfortable r+'ing all the new impls, so I'll tag with T-libs again.

@clarfonthey
Copy link
Contributor Author

For Travis, it looks like the errors are from a lack of Clone for the boxed types; I'm going to add those in there too. @.@

I perfectly understand the caution on the new impls; will be waiting for a response from libs. :)

@alexcrichton
Copy link
Member

Discussed during triage today, conclusion was that these were good to go.

@bors: r+

@bors
Copy link
Collaborator

bors commented Mar 14, 2017

📌 Commit 560944b has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Mar 15, 2017

⌛ Testing commit 560944b with merge 71d7b29...

bors added a commit that referenced this pull request Mar 15, 2017
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.
@bors
Copy link
Collaborator

bors commented Mar 15, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 71d7b29 to master...

@bors bors merged commit 560944b into rust-lang:master Mar 15, 2017
@clarfonthey clarfonthey deleted the box_to_buf branch March 15, 2017 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants