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

More methods for str boxes. (reduce Box<[u8]> ↔ Box<str> transmutes) #41258

Merged
merged 1 commit into from Apr 26, 2017

Conversation

Projects
None yet
9 participants
@clarcharr
Contributor

clarcharr commented Apr 12, 2017

This is a follow-up to #41096 that adds safer methods for converting between Box<str> and Box<[u8]>. They're gated under a different feature from the &mut str methods because they may be too niche to include in public APIs, although having them internally helps reduce the number of transmutes the standard library uses.

What's added:

  • From<Box<str>> for Box<[u8]>
  • <Box<str>>::into_boxed_bytes (just calls Into::into)
  • alloc::str (new module)
  • from_boxed_utf8 and from_boxed_utf8_unchecked, defined in alloc:str, exported in collections::str
  • exports from_utf8_mut in collections::str (missed from previous PR)
@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Apr 12, 2017

Collaborator

r? @brson

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

Collaborator

rust-highfive commented Apr 12, 2017

r? @brson

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

@clarcharr clarcharr changed the title from More methods for str boxes. to More methods for str boxes. (reduce Box<[u8]> ↔ Box<str> transmutes) Apr 12, 2017

@alexcrichton alexcrichton added the T-libs label Apr 13, 2017

@shepmaster

This comment has been minimized.

Show comment
Hide comment
@shepmaster

shepmaster Apr 14, 2017

Member

@brson's going to be globe-trotting for a bit, so...

r? @Kimundi

Member

shepmaster commented Apr 14, 2017

@brson's going to be globe-trotting for a bit, so...

r? @Kimundi

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Apr 18, 2017

Contributor

Thanks for the PR @clarcharr! Sorry for the delay - half of the team was on spring vacation. @Kimundi or some other reviewer will be looking at your PR soon.

Contributor

arielb1 commented Apr 18, 2017

Thanks for the PR @clarcharr! Sorry for the delay - half of the team was on spring vacation. @Kimundi or some other reviewer will be looking at your PR soon.

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Apr 18, 2017

Contributor

No problem! Whenever someone gets around to it. :)

Contributor

clarcharr commented Apr 18, 2017

No problem! Whenever someone gets around to it. :)

@Kimundi

This comment has been minimized.

Show comment
Hide comment
@Kimundi

Kimundi Apr 19, 2017

Member

Alright, finally got to it. :)

These seem like reasonable additions to me, and I don't see any issue with the code itself.

@bors: r+

Member

Kimundi commented Apr 19, 2017

Alright, finally got to it. :)

These seem like reasonable additions to me, and I don't see any issue with the code itself.

@bors: r+

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Apr 19, 2017

Member

@bors: r=Kimundi

Member

alexcrichton commented Apr 19, 2017

@bors: r=Kimundi

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Apr 19, 2017

Contributor

📌 Commit 112459d has been approved by Kimundi

Contributor

bors commented Apr 19, 2017

📌 Commit 112459d has been approved by Kimundi

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Apr 20, 2017

Contributor

☔️ The latest upstream changes (presumably #41411) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

bors commented Apr 20, 2017

☔️ The latest upstream changes (presumably #41411) made this pull request unmergeable. Please resolve the merge conflicts.

@frewsxcv

This comment has been minimized.

Show comment
Hide comment
@frewsxcv

frewsxcv Apr 20, 2017

Member

FYI, my PR #41295 just landed which made a couple organizational changes to the Unstable Book. Namely, features are now divided into 'language features' and 'library features' which are different directories, so you'll unfortunately have to address the conflicts. You should be able to just run ./x.py test src/tools/tidy to see what needs to be changed after rebasing/merging in master. Sorry for the inconvenience and if you need any help with that let me know! I can address the merge conflicts for you if you want.

Member

frewsxcv commented Apr 20, 2017

FYI, my PR #41295 just landed which made a couple organizational changes to the Unstable Book. Namely, features are now divided into 'language features' and 'library features' which are different directories, so you'll unfortunately have to address the conflicts. You should be able to just run ./x.py test src/tools/tidy to see what needs to be changed after rebasing/merging in master. Sorry for the inconvenience and if you need any help with that let me know! I can address the merge conflicts for you if you want.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Apr 20, 2017

Contributor

🔒 Merge conflict

Contributor

bors commented Apr 20, 2017

🔒 Merge conflict

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Apr 20, 2017

Contributor

Just rebased!

Contributor

clarcharr commented Apr 20, 2017

Just rebased!

Show outdated Hide outdated src/doc/unstable-book/src/SUMMARY.md
@@ -207,6 +207,7 @@
- [str_checked_slicing](library-features/str-checked-slicing.md)
- [str_escape](library-features/str-escape.md)
- [str_internals](library-features/str-internals.md)
- [str_box_extras](libaryr-features/str-box-extras.md)

This comment has been minimized.

@frewsxcv

frewsxcv Apr 20, 2017

Member

libaryr

😬

@frewsxcv

frewsxcv Apr 20, 2017

Member

libaryr

😬

This comment has been minimized.

@clarcharr

clarcharr Apr 20, 2017

Contributor

shhhhhhh

@clarcharr

clarcharr Apr 20, 2017

Contributor

shhhhhhh

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Apr 21, 2017

Contributor

(Rebased a second time and now it passes.)

Contributor

clarcharr commented Apr 21, 2017

(Rebased a second time and now it passes.)

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Apr 21, 2017

Member

Hm so looking this over, I fear that we're getting too aggressive with all of these conversions. For example functions like from_boxed_utf8 seem like you wouldn't actually wish to call them because in the case of a failure you lose the entire allocation.

Member

alexcrichton commented Apr 21, 2017

Hm so looking this over, I fear that we're getting too aggressive with all of these conversions. For example functions like from_boxed_utf8 seem like you wouldn't actually wish to call them because in the case of a failure you lose the entire allocation.

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Apr 21, 2017

Contributor

That makes sense! I can remove that function for now if that seems best.

Contributor

clarcharr commented Apr 21, 2017

That makes sense! I can remove that function for now if that seems best.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Apr 21, 2017

Member

Ok, let's remove that function.

Member

alexcrichton commented Apr 21, 2017

Ok, let's remove that function.

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Apr 21, 2017

Contributor

Removed for now. I also marked the alloc::str module itself as unstable.

Contributor

clarcharr commented Apr 21, 2017

Removed for now. I also marked the alloc::str module itself as unstable.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Apr 24, 2017

Member

Looks like Travis is failing?

Member

alexcrichton commented Apr 24, 2017

Looks like Travis is failing?

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Apr 24, 2017

Contributor

Hopefully it should be fixed once Travis finishes!

Contributor

clarcharr commented Apr 24, 2017

Hopefully it should be fixed once Travis finishes!

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Apr 25, 2017

Contributor

(Travis passes.)

Contributor

clarcharr commented Apr 25, 2017

(Travis passes.)

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Apr 26, 2017

Member

@bors: r=Kimundi

Member

alexcrichton commented Apr 26, 2017

@bors: r=Kimundi

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Apr 26, 2017

Contributor

📌 Commit c66c6e9 has been approved by Kimundi

Contributor

bors commented Apr 26, 2017

📌 Commit c66c6e9 has been approved by Kimundi

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Apr 26, 2017

Contributor

⌛️ Testing commit c66c6e9 with merge dad9814...

Contributor

bors commented Apr 26, 2017

⌛️ Testing commit c66c6e9 with merge dad9814...

bors added a commit that referenced this pull request Apr 26, 2017

Auto merge of #41258 - clarcharr:str_box_extras, r=Kimundi
More methods for str boxes. (reduce Box<[u8]> ↔ Box<str> transmutes)

This is a follow-up to #41096 that adds safer methods for converting between `Box<str>` and `Box<[u8]>`. They're gated under a different feature from the `&mut str` methods because they may be too niche to include in public APIs, although having them internally helps reduce the number of transmutes the standard library uses.

What's added:

* `From<Box<str>> for Box<[u8]>`
* `<Box<str>>::into_boxed_bytes` (just calls `Into::into`)
* `alloc::str` (new module)
* `from_boxed_utf8` and `from_boxed_utf8_unchecked`, defined in `alloc:str`, exported in `collections::str`
* exports `from_utf8_mut` in `collections::str` (missed from previous PR)
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Apr 26, 2017

Contributor

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

Contributor

bors commented Apr 26, 2017

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

@bors bors merged commit c66c6e9 into rust-lang:master Apr 26, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@bors bors referenced this pull request Apr 26, 2017

Closed

Gen herd cows #41506

@clarcharr clarcharr deleted the clarcharr:str_box_extras branch Apr 28, 2017

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