Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd methods to move ByteStrings underlying bytes #9684
Conversation
|
r? @KiChjang |
|
Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion. components/script/dom/bindings/str.rs, line 46 [r1] (raw file): Comments from the review on Reviewable.io |
|
Wow, this is pretty good work, thanks for your contribution! @bors-servo r+ |
|
|
Add methods to move ByteStrings underlying bytes Add methods to move the underlying `Vec<u8>` for `ByteString`. I saw this as at least two methods. One to "move and replace with and empty Vec<u8> (`bytes`), and one to take ownership of the whole object (`own_bytes`). I typically also don't like adding methods with out unit tests. If you think they're unnecessary, just let me know. As always, please let me know if you have any comments, critiques, or nits! Fixes: #9655 <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9684) <!-- Reviewable:end -->
|
|
| @@ -28,10 +29,21 @@ impl ByteString { | |||
| str::from_utf8(&vec).ok() | |||
| } | |||
|
|
|||
| /// Returns ownership of the underlying Vec<u8> the ByteString is | |||
| /// unusable afterwards as it has been moved | |||
This comment has been minimized.
This comment has been minimized.
KiChjang
Feb 18, 2016
Member
Takes ownership of the underlying Vec<u8>. The ByteString is unusable afterwards.
This comment has been minimized.
This comment has been minimized.
| @@ -28,10 +29,21 @@ impl ByteString { | |||
| str::from_utf8(&vec).ok() | |||
| } | |||
|
|
|||
| /// Returns ownership of the underlying Vec<u8> the ByteString is | |||
| /// unusable afterwards as it has been moved | |||
| pub fn own_bytes(self) -> Vec<u8> { | |||
This comment has been minimized.
This comment has been minimized.
KiChjang
Feb 18, 2016
Member
I'm thinking that this should be instead named into_bytes to be consistent with std namings.
This comment has been minimized.
This comment has been minimized.
dlrobertson
Feb 18, 2016
Author
Contributor
Ah! Good point! I thought about something like drain_bytes, but we'll go with into_bytes.
|
|
||
| /// Takes ownership of the underlying Vec<u8>. The ByteString is | ||
| /// unusable afterwards. | ||
| pub fn own_bytes(self) -> Vec<u8> { |
This comment has been minimized.
This comment has been minimized.
|
|
||
| /// Returns ownership of the underlying Vec<u8> and copies an empty | ||
| /// vec in its place | ||
| pub fn into_bytes(&mut self) -> Vec<u8> { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Add methods to move the underlying Vec<u8> for ByteString.
Convert traditional unwrapping of the tuple struct ByteString to self.0
|
@bors-servo r+ |
|
|
Add methods to move ByteStrings underlying bytes Add methods to move the underlying `Vec<u8>` for `ByteString`. I saw this as at least two methods. One to "move and replace with and empty Vec<u8> (`bytes`), and one to take ownership of the whole object (`own_bytes`). I typically also don't like adding methods with out unit tests. If you think they're unnecessary, just let me know. As always, please let me know if you have any comments, critiques, or nits! Fixes #9655 <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9684) <!-- Reviewable:end -->
|
|
|
@bors-servo retry |
Add methods to move ByteStrings underlying bytes Add methods to move the underlying `Vec<u8>` for `ByteString`. I saw this as at least two methods. One to "move and replace with and empty Vec<u8> (`bytes`), and one to take ownership of the whole object (`own_bytes`). I typically also don't like adding methods with out unit tests. If you think they're unnecessary, just let me know. As always, please let me know if you have any comments, critiques, or nits! Fixes #9655 <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9684) <!-- Reviewable:end -->
|
|
|
@bors-servo retry |
|
|
|
|
dlrobertson commentedFeb 17, 2016
Add methods to move the underlying
Vec<u8>forByteString.I saw this as at least two methods. One to "move and replace with and empty Vec (
bytes), and one to take ownership of the whole object (own_bytes). I typically also don't like adding methods with out unit tests. If you think they're unnecessary, just let me know.As always, please let me know if you have any comments, critiques, or nits!
Fixes #9655