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 support for const length arrays ([u8; N]) #782

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ryankurte
Copy link

@ryankurte ryankurte commented Dec 14, 2022

minimal addition to implement prost::encode::BytesAdapter over [u8; N] (alongside Vec<u8> and Bytes<u8>.

@VladimirBramstedt
Copy link

the problem is that there is no way to express that in protobuf.
if you have [u8;20] and the field you are parsing only contains 10 bytes? what do you do? you'd need to add some parameter (len) to track how much of that buffer is filled, etc... at which point you want heapless::Vec<T;N> (which does also implement Default), which the maintainers did not want to add...

@ryankurte
Copy link
Author

the problem is that there is no way to express that in protobuf.

if you have [u8;20] and the field you are parsing only contains 10 bytes? what do you do?

it seems pretty reasonable to me to encode a fixed length field as variable length in the proto, and to fail on the decoding of a mismatched length, as one would with any other invalid or unmatched message / field (though this PR does not yet have that behaviour).

at which point you want heapless::Vec<T;N>

this addresses an issue with prost::Message not functioning over otherwise readily serializable type with explicitly sized fields, i specifically do not want to replace these with variable length versions that don't make sense from the application side. whether or not this is the ideal construction of application and serialisation objects, it is a problem that exists for users of core language primitives.

@VladimirBramstedt
Copy link

it seems pretty reasonable to me to encode a fixed length field as variable length in the proto, and to fail on the decoding of a mismatched length, as one would with any other invalid or unmatched message / field (though this PR does not yet have that behaviour).

Alright, but that behaviour is the key point, in my opinion.
you are not merely adding the ability to use arrays as byte containers, you are also stating here that decoding would fail if the size doesnt match. without that assertion this PR makes no sense, and that assertion isnt in the PR as is. is this a draft PR then?

@ryankurte ryankurte changed the title add support for const length arrays ([u8; N]) WIP: add support for const length arrays ([u8; N]) Dec 16, 2022
@ryankurte
Copy link
Author

updated PR to propagate errors, expose BytesAdapter. it'd be great to know if there was any interest in merging something like this prior to spending more effort on it?

make BytesAdapter externally implementable, propagate errors from BytesAdapter decode fn
@ryankurte ryankurte force-pushed the feature/const-arrays branch from 086fdbd to 753aa7a Compare January 3, 2023 23:14
@ryankurte ryankurte changed the title WIP: add support for const length arrays ([u8; N]) add support for const length arrays ([u8; N]) Jan 11, 2023
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.

2 participants