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

ByteBuf: Vec<u8, N> with efficient serde (+ some Vec methods) #164

Closed

Conversation

nicolas-solokeys
Copy link

What's this?

  • new type ByteBuf that is a Vec<u8, N> with different / more efficient serde (as bytes). Inherits most functionality via its Deref to Vec.
  • some additional methods (into_vec, to_vec, try_to_vec, from_writer, insert_slice_at, inherent as_slice/as_mut_slice, etc.)
  • wrapper types allowing one to perform efficient serde on slices and byte vectors without using the ByteBuf type
  • addition of some uDebug implementations, dependency on ufmt instead of ufmt-write (otherwise there would be too many cargo features)

This is a melange of existing heapless, serde_bytes and std code. I've found it very useful to have a standard "extendable bytes" type (merging [u8; N] and Vec<u8, N>), so I believe this is also of independent interest as shared type.

I left out the Bytes type from serde_bytes as I've found no use for it; could be added.

I would prefer using the name Bytes for ByteBuf, but I think that would be confusing to anyone used to the serde_bytes ByteBuf.

MSRV: insert_slice_at would profit from copy_within, which stabilised in 1.37 while the MSRV is 1.36. I assume we don't want to bump the MSRV.

@korken89
Copy link
Contributor

Thanks for the PR! I'll put this on my review list.

When it comes to MSRV, I see no problem in bumping it as long as we stay within stable. I don't think we have a rule on minor or major release for new MSRV.
Hopefully the minimal const generics support will come in 2020, and with that we will nuke MSRV anyways.

Do you have comments on this @japaric ?

@japaric
Copy link
Member

japaric commented Jul 28, 2020

Do you have comments on this @japaric ?

I must say I'm not super thrilled about adding a newtype that could well live in a separate crate (there are no orphan rules issues around this), specially given that (a) serde is an opt-in (optional) feature of this crate and (b) this works around the lack of specialization on stable.

I would personally prefer if this code lived in a separate crate, like serde_bytes does, to not increase the maintenance burden (+1.2K line diff) of this crate.

@nickray
Copy link
Contributor

nickray commented Aug 28, 2020

I'm not sure whether and how to proceed on this, as the feedback is negative and does not, despite being open for quite a long time, review the details of the PR at all.

I'd like to emphasize that there are three separate parts:

  • extensions to heapless::Vec, nearly all of them adding methods that std::Vec has. In my practical experience, this increases the ergonomics of using Vec a lot.
  • serialization of ByteBuf as "bytes" type (vs. collection/array/...), which is independent of specialization, as many formats distinguish between the two serializations, both of which are applicable to the type.
  • the serde_as_bytes wrapper code, allowing serializing a Vec as bytes (vs. converting to the ByteBuf type first)

I'd very much like to upstream at least the first part, and hopefully the second part as well. If "line count" is a criterion, removing the third part from this PR might be an option. However, with most of that code being simple type gymnastics, I don't quite see the claimed maintenance burden (especially vs. keeping an external crate in sync), and from the point of view of serde being an integral part of most real life programming, I feel there are good practical reasons to have it in a shared code base.

@nickray
Copy link
Contributor

nickray commented Mar 2, 2021

Closing this for now, as a) I've come to agree heapless-bytes probably doesn't belong here, and b) neither does ufmt :)

Still hope to get more std::Vec-inspired methods in the heapless::Vec API, and will try to revisit just that. As it adds useful expressivity / ergonomics. Best for/after an 0.7 with const-generics.

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