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

Add an into vec implementation? #154

Closed
Sushisource opened this issue Feb 18, 2019 · 3 comments · Fixed by #378
Closed

Add an into vec implementation? #154

Sushisource opened this issue Feb 18, 2019 · 3 comments · Fixed by #378

Comments

@Sushisource
Copy link

Hi there. Like Prost a lot. I find that by far the most common case for me when encoding is

  1. Make a new empty vector
  2. Immediately encode into it.

I absolutely understand why the encode function works the way it does, but given how common the case I just mentioned is, seems worth making this even easier on the caller. Namely:

let encoded: Vec<u8> = message.into()

This would be super. If you like the idea, I'm happy to contribute a PR (admittedly, not a whole lot to do here 😄 )

Thanks!

@rubdos
Copy link
Contributor

rubdos commented Oct 26, 2020

Hey! Just jumping in here. Into is typically used for cheap, infallible conversions. Allocating does not (in my opinion) fall in that category. However, I found it tiresome too to write Vec::with_capacity(encoded_len()) the whole day, so I made #378: encode_to_vec() and encode_length_delimited_to_vec().

I think that will solve your underlying issue well, so I've marked this issue as closed by that PR. If you disagree, let me know, I'll unmark it.

@Sushisource
Copy link
Author

@rubdos Awesome! That's perfect, and great point about this maybe not being the best for Into.

@wchargin
Copy link
Contributor

In addition to admitting the with_capacity optimization, such a
dedicated encode_to_vec method can return Vec<u8> rather than
Result<Vec<u8>>, which saves callers from having to expect with a
comment indicating why this is always safe.

I see that the implementation in #378 does so, so imho this is another
good reason to support this feature:

  • more concise
  • expresses programmer intent better
  • works in an expression context
  • more efficient due to with_capacity
  • more precise return type

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 a pull request may close this issue.

3 participants