Conversation
|
I like this concept a lot, but I have a vague memory I had a discussion about this with @ZoeyR a while back and we decided not to do this. We'll get back to you |
|
I had a discussion with @ZoeyR and we decided that this is a useful feature and we're willing to break open the API for this. Could you get this in a state where the CI succeeds? Specifically test As for the TODOs:
I like writing out things fully, so
I maintain virtue so feel free to make a PR to that
Let's start with the minimum effort you need to do to get this merged, we can enhance the macros later Thanks a lot for your proposal! |
|
That's great to hear. Thanks! I'll work on it as soon as I'm back from vacation. |
|
Pinging @branchseer we'd love to have this in bincode 2 |
Ah sorry I totally forgot! Will work on it this weekend! |
|
Hi @VictorKoenders, let's start with bincode-org/virtue#90. It's for generating |
62f8701 to
45586f4
Compare
45586f4 to
e74149d
Compare
|
It seems the derive macros aren't always doing the right thing. If I derive Decode, it fails because the generated code fails with: |
|
Bincode derive should be outputting the files in |
|
Would be great if we can get this merged before releasing 2.0 |
|
Closing this since I pulled in the changes in #749 |
|
Congratulations on the release! What do you think about making the unit type the default for the |
Why
With the current
Decode/BorrowDecode, it's impossible to implement them on a type that borrows something other than the input data. A concrete example would be aVec<'bump, T: 'bump>which is allocated by an arena allocator.How
This PR introduces the following changes:
decode_from_slice_with_ctx<Ctx, D: de::Decode<Ctx>, C: Config>(src: &[u8], config: C, ctx: Ctx). In that arenaVecexample, the decode context would be the allocator&Bump.DecoderImpland exposed toDecodeimplements viaDecoder::ctxmethod.Decodetrait is changed toDecode<C>, which allows types to implementDecodeonly under some specific context. (Vec<'bump, T: 'bump>would implementDecode<&'bump Bump>)Decodeis changed to generatingimpl<Ctx> Decode<Ctx> for ...instead ofimpl Decode for .... A container attributedecode_contextis added to allow derivingDecodewith a concrete context type. (for example#[bincode(decode_context = "&'bump Bump")])BorrowDecoder.You can see how these changes work together in
tests/ctx.rs.I know this is a big change and would break existing
2.0.0users. But it's stillrcso I guess it's not too late to discuss it? ;)Alternative Design
Leaves
(Borrow)Decodeuntouched, and add(Borrow)DecodeWithContext<C>.Pros:
(Borrow)Decode.Con:
TODOs
This PR hasn't been polished to a mergeable state yet. I will do that if the overall design is accepted.
impl<Ctx>. This would need a separate PR.DecodeallowsCtxto be either a concrete type or totally unbounded. How about allowingCtx: ...? Is it useful enough to be implemented?