serde-rs / bytes

Wrapper types to enable optimized handling of &[u8] and Vec<u8>
Apache License 2.0
306 stars 37 forks source link

Implement ToOwned on Bytes #10

Closed RReverser closed 5 years ago

RReverser commented 5 years ago

Per common convention, Bytes should implement ToOwned that converts them to ByteBuf.

In particular, this would allow easily using Cow<Bytes> in contexts where you want either borrowed or owned data.

dtolnay commented 5 years ago

Seems reasonable! I would be prepared to consider a PR for this.

RReverser commented 5 years ago

sigh looks like this won't work due to coherence (as usual in Rust) :(

ToOwned can't be implemented on types that have Clone, and Bytes without Clone can't be Copyable either, and losing these two seems bad.

Standard library doesn't have this issue with &[u8] / &str / etc. because these live in the same crate as ToOwned and so can be specialised, even though these types implement Clone and Copy too.

dtolnay commented 5 years ago

We probably want to change Bytes to a dst for this and other reasons.

pub struct Bytes {
    bytes: [u8],
}
RReverser commented 5 years ago

That would work (although would be a breaking change). If you're open to a PR, I can do both together.

RReverser commented 5 years ago

What is the target Rust version for serde_bytes though? I believe the above can be technically safe only with #[repr(transparent)] which is stable in Rust 1.28+, but I remember from other discussion that you're supporting older versions of Rust too?

dtolnay commented 5 years ago

repr(C) is sufficient in this case because we don't need FFI guarantees. But major versions are allowed to raise the rustc requirement.

RReverser commented 5 years ago

To make sure we're on the same page - we're both talking about safety of transmute::<&[u8], &Bytes> right?

If so, I'm not sure repr(C) is enough. It would be enough for thin pointers, because then we only care that start address of the first field == address of the struct itself, but fat pointers have own internal representation that is AFAIK not guaranteed to be compatible without repr(transparent).

dtolnay commented 5 years ago

transmute::<&[u8], &Bytes> only requires repr(C). The guarantees of repr(transparent) are the same as repr(C) plus one extra guarantee about how the struct is seen by calling conventions of extern functions, particularly in calling conventions that treat all struct types differently from primitive types.

Transmuting is not the same as FFI so repr(transparent) vs repr(C) has no bearing on transmute (except when transmuting FFI function pointers).

RReverser commented 5 years ago

Huh, I thought they can be different in regards to memory layout too (especially around padding after last field), but apparently not. I stand corrected.

Do you want to perform this rewrite to DST yourself or would you rather prefer a PR?

dtolnay commented 5 years ago

I won't get to it soon so a PR would be better. Thanks!

RReverser commented 5 years ago

This is somewhat bigger change, so it will take me some time to get around to it too, but I'll try to revisit this in a week or so.

RReverser commented 5 years ago

Should ByteBuf in that case be changed to deref to Bytes instead of [u8]?

dtolnay commented 5 years ago

Fixed in 0.11.0.