tokio-rs / prost

PROST! a Protocol Buffers implementation for the Rust Language
Apache License 2.0
3.78k stars 489 forks source link

feat: add str encoding helper #978

Closed morrisonlevi closed 6 months ago

morrisonlevi commented 7 months ago

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.

morrisonlevi commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

cargo test runs fine on my branch.

morrisonlevi commented 7 months ago

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

guilload commented 7 months ago

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