Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Mar 15, 2017
Merged

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
Collaborator

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

bors commented Mar 14, 2017

📌 Commit 560944b has been approved by alexcrichton

@bors
Copy link
Contributor

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
Contributor

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