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

implement Write trait for BytesMut per issue #425 #478

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

rich-murphey
Copy link

There is some discussion of this in #425.

This implements Write for BytesMut exactly as is done for Vec here:

https://doc.rust-lang.org/src/std/io/impls.rs.html#370-402

taiki-e
taiki-e previously approved these changes Feb 7, 2021
Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@carllerche
Copy link
Member

Could we add some tests?

@sfackler
Copy link
Contributor

sfackler commented Feb 7, 2021

Because BufMut already implements fmt::Write, this change will break any code that uses write!() with a BytesMut while both fmt::Write and io::Write are imported.

@taiki-e taiki-e dismissed their stale review February 7, 2021 17:45

Sorry, I missed conflict with fmt::Write.

@carllerche
Copy link
Member

@sfackler good catch!

we should add a test to cover this edge case.

@rich-murphey
Copy link
Author

At the moment, I'm not sure how to proceed. I don't know how to avoid breaking write!() as @sfackler said, but wow, I'm so glad he caught it.

@carllerche
Copy link
Member

I don't see a way to proceed with this. It isn't great, but one can always use BufMut::writer() to adapt.

I would appreciate a test that demonstrates the ability to call write! with both mentioned traits in scope. You can reuse this PR or submit a new one.

@sfackler
Copy link
Contributor

sfackler commented Feb 8, 2021

I think you could in theory add an inherent write_fmt method directly on impl BytesMut since that would take precedence over either of the trait methods. It's a bit gross though.

@rich-murphey
Copy link
Author

Thanks so much for the directions @sfackler and @carllerche. I'll write some tests so we can evaluate them.

@rich-murphey
Copy link
Author

I tried adding write_fmt() directly on impl BytesMut, and still saw compiler errors when write!() saw both io::Write and fmt::Write impls.

So here's another option. This adds a BytesMutExt trait that implements io::Write.

It's a bit awkward to use though:. The successful test (included) looks like this:

#[test]
fn fmt_write_bytesmut_with_both_write_in_scope_test() {
    use bytes::ext::BytesMutExt;
    use bytes::BytesMut;
    #[allow(unused_imports)]
    use std::fmt::Write as FmtWrite;
    use std::io::Write as IoWrite;
    let mut buf = BytesMut::with_capacity(64);
    write!(&mut buf as &mut dyn BytesMutExt, "hello").ok();
    assert_eq!(&buf[..], b"hello");
}

This works, but it's awkward. I'm not sure this is helpful at all.

Please let me know if I've misunderstood the idea of having a write_fmt() directly in impl BytesMut.

And if you think this is a dead horse, that 's ok too.

@scottlamb
Copy link

Found my way here from the issue. Did you consider something like the following?

write!(buf.as_io_write(), "hello")

I think this is more readable than write!(&mut buf as &mut dyn BytesMutExt, "hello") and doesn't use dynamic dispatch.

The problem I have with the current BufMut::writer is that it consumes the BytesMut which doesn't work when you don't own the it. Eg in a tokio_util::codec::Encoder I have to std::mem::replace out the BytesMut then put it back afterward:

https://github.com/scottlamb/retina/blob/265b6b94901d02b03e51d2b6f3338de899077687/src/lib.rs#L360-L363

fn as_io_write(&mut self) -> IoWriteRef<'_> (or similar) wouldn't have this problem.

@Darksonn
Copy link
Contributor

You could do write!((&mut buf).writer(), "hello"), still I do see the point.

@scottlamb
Copy link

scottlamb commented Jun 29, 2021

Oh, I didn't think of that. Thanks for the tip! Actually, I would have been perfectly happy if that example were just in writer()'s docstring.

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.

6 participants