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

Jump to bottom

Conversions between slices and boxes #39438

Merged
merged 1 commit into from Feb 10, 2017
Merged

Conversions between slices and boxes #39438

merged 1 commit into from Feb 10, 2017

Conversation

clarfonthey
Copy link
Contributor

@clarfonthey clarfonthey commented Feb 1, 2017 •
edited

This allows conversion for Copy slices, str, and CStr into their boxed counterparts.

This also adds the method CString::into_boxed_c_str.

I would like to add similar implementations for OsStr as well, but I have not figured out how.

@rust-highfive
Copy link
Collaborator

r? @aturon

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

@aturon
Copy link
Member

aturon commented Feb 1, 2017

Since these impls are instantly stable, we need the full libs team to sign off.

Personally, I think that we will continue growing such impls until the end of time. Due to trait coherence, std is the only place such impls can live. So I'm fine continuing to sign off on such PRs.

@rfcbot fcp merge

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

aturon commented Feb 1, 2017

Whoops, didn't have the libs team assigned first, trying again:

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Feb 1, 2017 •
edited by Kimundi

Team member @aturon has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@clarfonthey
Copy link
Contributor Author

Reminder to self for later: also add versions for CStr and OsStr

@alexcrichton
Copy link
Member

@clarcharr the compile failures here seem worrisome, they may want to be fixed!

@clarfonthey
Copy link
Contributor Author

clarfonthey commented Feb 4, 2017 •
edited

@alexcrichton I'll fix those soon; I know what's wrong.

Instead of using Vec and String I'll have to do it manually.

@clarfonthey clarfonthey changed the title Implement From<&str> and From<&[T]> for Box Conversions between slices and boxes Feb 5, 2017
@clarfonthey
Copy link
Contributor Author

Waiting on tests, but this should work now. I also added an implementation for CStr and added a new method, CString::into_boxed_c_str, which will need a tracking issue once I get an informal nod that such a method is okay to add.

@clarfonthey clarfonthey force-pushed the box_from branch 12 times, most recently from 69e4ee6 to 7b71cd1 Compare February 6, 2017 18:25
@alexcrichton
Copy link
Member

@clarcharr perhaps the CStr parts could be left out for a future PR? Unfortunately trait impls are instantly stable today and we don't have much other precedent with that type and DST impls, so we may just wish to conservatively tackle it in a separate PR

@clarfonthey
Copy link
Contributor Author

Sounds good to me! I'll split them out and open another PR.

@clarfonthey clarfonthey mentioned this pull request Feb 6, 2017
Merged
@clarfonthey clarfonthey closed this Feb 8, 2017
@clarfonthey clarfonthey deleted the box_from branch February 8, 2017 03:40
@clarfonthey
Copy link
Contributor Author

@malbarbo That's because it's more efficient if the type implements Copy, but I could make a more general version using impl specialisation. If you think it's worth having, feel free to open an issue and I might get around to it!

@malbarbo
Copy link
Contributor

malbarbo commented Feb 8, 2017

@clarcharr You can use Vec::from_iter, it is already specialized for copy types https://doc.rust-lang.org/stable/src/collections/vec.rs.html#1499-1670

@clarfonthey
Copy link
Contributor Author

@malbarbo right now that'll work for .to_vec().into_boxed_slice(), but not directly like in these conversions. I'll take a look at it in my other PR.

@clarfonthey clarfonthey restored the box_from branch February 10, 2017 06:17
@clarfonthey
Copy link
Contributor Author

So apparently this didn't get merged? Did I miss something?

@clarfonthey clarfonthey reopened this Feb 10, 2017
@bors
Copy link
Contributor

bors commented Feb 10, 2017

⌛ Testing commit 550373b with merge 8482c4b...

@bjorn3
Copy link
Member

bjorn3 commented Feb 10, 2017

@clarcharr The rollup pr failed, so you will have to wait till the next rollup. (Got a pr of mine merged after 6 rollup prs)

@bors
Copy link
Contributor

bors commented Feb 10, 2017

💔 Test failed - status-appveyor

@alexcrichton
Copy link
Member

alexcrichton commented Feb 10, 2017 via email

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 10, 2017
@frewsxcv
Rollup merge of rust-lang#39438 - clarcharr:box_from, r=alexcrichton
2376e42 
Conversions between slices and boxes

This allows conversion for `Copy` slices, `str`, and `CStr` into their boxed counterparts.

This also adds the method `CString::into_boxed_c_str`.

I would like to add similar implementations for `OsStr` as well, but I have not figured out how.
@frewsxcv frewsxcv mentioned this pull request Feb 10, 2017
Closed
@bors
Copy link
Contributor

bors commented Feb 10, 2017

⌛ Testing commit 550373b with merge 16f5492...

@bors
Copy link
Contributor

bors commented Feb 10, 2017

💔 Test failed - status-travis

@bjorn3
Copy link
Member

bjorn3 commented Feb 10, 2017

dist-mips-linux:

[...]

[ 83%] Built target LLVMMSP430Info
Scanning dependencies of target LLVMMSP430Desc
[ 83%] Building CXX object lib/Target/MSP430/MCTargetDesc/CMakeFiles/LLVMMSP430Desc.dir/MSP430MCTargetDesc.cpp.o


No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

The build has been terminated

@alexcrichton
Copy link
Member

alexcrichton commented Feb 10, 2017 via email

@bors
Copy link
Contributor

bors commented Feb 10, 2017

⌛ Testing commit 550373b with merge 99b6677...

@bors
Copy link
Contributor

bors commented Feb 10, 2017

💔 Test failed - status-travis

@alexcrichton
Copy link
Member

@bors: retry

  • flaky thread sanitizer test? (guessing)

@japaric does that look familiar? We may have to disable the test if it ends up being flaky :(

@bors
Copy link
Contributor

bors commented Feb 10, 2017

⌛ Testing commit 550373b with merge 2425b22...

bors added a commit that referenced this pull request Feb 10, 2017
@bors
Auto merge of #39438 - clarcharr:box_from, r=alexcrichton
2425b22 
Conversions between slices and boxes

This allows conversion for `Copy` slices, `str`, and `CStr` into their boxed counterparts.

This also adds the method `CString::into_boxed_c_str`.

I would like to add similar implementations for `OsStr` as well, but I have not figured out how.
@bors
Copy link
Contributor

bors commented Feb 10, 2017

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

@bors bors merged commit 550373b into rust-lang:master Feb 10, 2017
@japaric
Copy link
Member

japaric commented Feb 12, 2017

@alexcrichton Sadly, no. I haven't seen the thread sanitizer not catch that data race before. I'll test locally to see if I can repro.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@aturon aturon

Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@clarfonthey @rust-highfive @aturon @rfcbot @alexcrichton @bors @malbarbo @bjorn3 @japaric