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

[WIP] Implement everything for 0.5 #210

Closed
wants to merge 6 commits into from
Closed

[WIP] Implement everything for 0.5 #210

wants to merge 6 commits into from

Conversation

YetAnotherMinion
Copy link
Contributor

@YetAnotherMinion YetAnotherMinion commented Jun 1, 2018

I am working off of the list of issues under 0.5 milestone.

This should address: #158, #75, #139, #196, #74

Changelog

  • Renames the Buf::take method to Buf::limit to better describe what that method does. The rename was necessary to prevent ambiguity with BytesMut::take. I renamed Take to Limit to match the constructor method name.

  • Removed Buf impl for Cursor and replaced it with impl Buf for Bytes

  • Removed Buf impl for Cursor<BytesMut> and replaced it with impl Buf for BytesMut

  • Renamed Iter to IntoIter to match naming convention of rust where iterators that move the collection are named IntoIter

  • Adds impl<T: Buf> IntoIterator for T that constructs a bytes::buf::IntoIter struct.

  • Removs iter method from Buf trait. The existing behavior with respect to ownership matched the convention of into_iter instead of iter. The builtin IntoIterator trait already provides for into_iter, so I decided to avoid ambiguity by just removing Buf::iter instead of a rename. It is up to each container implementing Buf to implement iter efficiently if possible.

  • Adds Bytes::iter which returns slice::Iter.

  • Adds BytesMut::iter which returns slice::Iter.

  • Adds Bytes::copy_from_slice(src: &'a [u8]) which constructs a new

  • Removed impl<'a> From<&'a [u8]> for Bytes and replaced with impl From<&'static [u8]> for Bytes which does not copy the data.

  • Adds IntoBuf::len which returns the size of the buffer that would be created by calling into_buf() on self.

@seanmonstar
Copy link
Member

Thanks for taking this on!

  • As a suggestion, I think changing to Buf::limit(n) may be clearer. I'm not a fan of Iterator::take(n), since it sounds like an immediate action, but is actually lazy. Buf::prefix(n) might imply that n bytes are being prefixed before this buffer.
  • There was a reason for Buf::iter, which was to provide an Iterator as long as you implement Buf. This could be helpful for composite buffer types. However, I agree with your decision. There isn't a need to provide an Iter implementation over Buf, it is more efficient for types to implement their own iterators if they can.

@YetAnotherMinion
Copy link
Contributor Author

@seanmonstar, you mentioned in
#75 (comment) that Bytes and BytesMut should implement IntoIterator. My current patch completely removed IntoIterator impl on these types in favor of a blanket impl for T: Buf.

Is this design acceptable?

@seanmonstar
Copy link
Member

@YetAnotherMinion I think Bytes and BytesMut probably should implement IntoIterator, since it's needed to allow a_bytes_mut.extend(some_other_bytes).

@YetAnotherMinion
Copy link
Contributor Author

@seanmonstar They do implement IntoIterator through their implementation of Buf.

Apply this patch to this PR branch to see it working.

diff --git a/tests/test_chain.rs b/tests/test_chain.rs
index 8877258..42a16eb 100644
--- a/tests/test_chain.rs
+++ b/tests/test_chain.rs
@@ -48,6 +48,15 @@ fn iterating_two_bufs() {
 }
 
 #[test]
+fn dirty_hack() {
+    let mut a = BytesMut::from(&b"hello"[..]);
+    let b = Bytes::from(&b"world"[..]);
+    a.extend(b);
+
+    assert_eq!(a.bytes(), &b"helloworld"[..]);
+}
+
+#[test]
 fn vectored_read() {
     let a = Cursor::new(Bytes::from(&b"hello"[..]));
     let b = Cursor::new(Bytes::from(&b"world"[..]));

@seanmonstar
Copy link
Member

Oh OK, I get it! That's probably good then.

@YetAnotherMinion YetAnotherMinion changed the title [WIP] removes impl IntoBuf for Cursor<Self>, impl Buf for Bytes, BytesMut, refactor iterators [WIP] Implement everything for 0.5 Jun 6, 2018
@arthurprs
Copy link
Contributor

Removed impl<'a> From<&'a [u8]> for Bytes and replaced with impl From<&'static [u8]> for Bytes which does not copy the data.

Is this really a good idea?

@YetAnotherMinion
Copy link
Contributor Author

@arthurprs see #139 for justification.

Additionally, I would not worry about this PR, it has been 5 months with no activity which means there is no business value in these proposed changes.

@arthurprs
Copy link
Contributor

While I understand the reasoning this severely limits Byte as a Vec replacement, like backing a https://crates.io/crates/string

@carllerche
Copy link
Member

@YetAnotherMinion Sorry for the radio silence, it was not cool on my part to drop the ball. This PR is great work and very helpful.

The 0.5 release delay is mostly due to an attempt to coordinate a release with Tokio 0.2 release. Tokio includes Bytes as a public dependency in core crates like tokio-io, which means that users of Tokio would have to stay on 0.4.

0.5 represents a cleanup pass and, as such, doesn't really have a reason to ship on its own. Once Tokio 0.2 gets closer (which I hope to start tracking better), we can revive this PR.

@seanmonstar
Copy link
Member

Sorry for it taking so long, we're now actively working on 0.5 things. I've started splitting this PR into specific features, such as #260. I'll close once all have been merged.

@YetAnotherMinion
Copy link
Contributor Author

Hey, thanks for picking this stuff up again. I ended up using a private fork at work and moved on. It will be nice to switch back over coming soon. If there are still some other issues for 0.5 that need help, I am happy to pitch in.

@seanmonstar
Copy link
Member

We've released 0.5!

@seanmonstar seanmonstar closed this Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants