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: add str encoding helper #978

Closed
wants to merge 1 commit into from

Conversation

morrisonlevi
Copy link

This allows users to encode structs that have types like &str, Box<str>, Cow<str>, etc if they are willing to get their hands dirty. This more efficient than creating a parallel type that holds a String, copying it over, just for the purpose of encoding it, then throwing it away.

Ideally we'd support types like Cow<'a, str> directly but there are plenty of issues and challenges with that. I am willing to get my hands dirty, but I prefer having this helper in prost rather than copying the relevant bits into my own project.

This allows users to encode types like `&str`, `Box<str>`. This can
be more efficient than creating a String just for the purpose of
encoding it.
@morrisonlevi
Copy link
Author

Failure is:

error: package `half v2.3.1` cannot be built because it requires rustc 1.70 or newer, while the currently active rustc version is 1.65.0
Either upgrade to rustc 1.70 or newer, or use
cargo update -p half@2.3.1 --precise ver
where `ver` is the latest version of `half` supporting rustc 1.65.0
Error: Process completed with exit code 101.

This PR didn't touch any of that stuff, obviously. In order to run tests locally, I had to use Rust 1.70.

@guilload
Copy link

guilload commented Feb 7, 2024

Hello @morrisonlevi,

I'm interested in this feature, too, for str and [u8] slices. Two questions:

  1. Why not rewrite string::encode directly as fn encode<S, B>(tag: u32, value: &impl AsRef<str>, buf: &mut B)?
  2. Would you consider doing the same thing for bytes?

For bytes, all we have to do is:

  1. Add an AsRef<[u8]> constraint on BytesAdapter: trait BytesAdapter: Default + Sized + AsRef<[u8]> + 'static
  2. Remove BytesAdapter::append_to
  3. Rewrite bytes::encode as:
pub fn encode<A, B>(tag: u32, value: &A, buf: &mut B)
where
        A: AsRef<[u8]>,
        B: BufMut,
    {
        let bytes = value.as_ref();
        encode_key(tag, WireType::LengthDelimited, buf);
        encode_varint(bytes.len() as u64, buf);
        buf.put_slice(bytes);
    }

@morrisonlevi
Copy link
Author

When I tried changing encode directly, tests would fail because of mismatches in expected types in a bunch of test helpers. It was easier to add this helper and keep the tests green.

@guilload
Copy link

guilload commented Feb 7, 2024

cargo test runs fine on my branch.

@morrisonlevi
Copy link
Author

@guilload If your tests are passing, feel free to open a PR and I'll close this one.

@guilload
Copy link

guilload commented Feb 9, 2024

Ok. I opened #979. CI is failing because of the MSRV issue, but tests are passing locally.

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.

None yet

2 participants