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

feat: encode string and bytes slices #979

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

guilload
Copy link

@guilload guilload commented Feb 9, 2024

This PR adds the ability to encode &str and &[u8] using string::encode and bytes::encode. This is useful for users writing custom Message implementations who want to avoid unnecessary allocations.

@guilload guilload force-pushed the guilload/encode-str-and-bytes-slices branch 2 times, most recently from 09db470 to ce9de1c Compare February 9, 2024 23:01
@LucioFranco
Copy link
Member

Can you provide some motivation for this change

@guilload
Copy link
Author

@morrisonlevi provided some context in his original PR: #978.

My use case is similar. I recently wrote a few custom Message implementations for types with some fields for which I can obtain a &str or &[u8] for free but not a String or Vec<u8> without allocating.

@guilload guilload force-pushed the guilload/encode-str-and-bytes-slices branch from ce9de1c to d61c094 Compare February 16, 2024 17:04
@guilload
Copy link
Author

@caspermeijn, is there any chance you can look into this PR?

Copy link
Collaborator

@caspermeijn caspermeijn left a comment

Choose a reason for hiding this comment

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

This PR lacks a description. I believe you want to expand the types that are accepted by string::encode and bytes::encode.


pub trait BytesAdapter: Default + Sized + 'static {
pub trait BytesAdapter: Default + Sized + AsRef<[u8]> + 'static {
Copy link
Collaborator

Choose a reason for hiding this comment

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

BytesAdapter is not used anymore for encode, but it seems to still be used for encoded_len. Will this mismatch cause issues for other people?

@@ -794,13 +794,15 @@ macro_rules! length_delimited {
pub mod string {
use super::*;

pub fn encode<B>(tag: u32, value: &String, buf: &mut B)
pub fn encode<S, B>(tag: u32, value: &S, buf: &mut B)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like to see tests to guarantee String can still be used on this function.

And you need to add tests for your new use case, to make sure it is not broken in the future.

}

pub mod bytes {
use super::*;

pub fn encode<A, B>(tag: u32, value: &A, buf: &mut B)
where
A: BytesAdapter,
A: AsRef<[u8]>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like to see tests to guarantee impl BytesAdapter, Bytes and Vec<u8> can still be used on this function.

And you need to add tests for your new use case, to make sure it is not broken in the future.

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.

3 participants