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 initial support for heapless #4

Closed
wants to merge 3 commits into from

Conversation

chrysn
Copy link

@chrysn chrysn commented Oct 30, 2024

The common embedded string type heapless::String is not supported in minicbor; neither are other common heapless types. Its implementation largely follows that of the alloc::…::String, although it takes a slightly different choice for encoding by fully deferring to the deref'd str.

Without introducing extra wrappers, this can only be done either in heapless or here. While heapless does implement serialization for serde, following the pattern of "smaller crate provides implementations for larger crate" and "unstable crate provides implementations for unstable crate", this would best sit here. (heapless is also unstable, but rather slow in breaking things.)

This PR adds support for heapless::String only. It is designed for early review as a test balloon – if the way this is done is fine for you, I'd add at least heapless::Vec (following alloc::…::Vec), if you prefer already in this PR. There are more types that could be expressed (heapless itself provides serde implementations also for its BinaryHeap, IndexSet, LinearMap and IndexMap), but as I have no experience with them, I'd defer them to implementation as needed, unless you insist on full coverage.

@chrysn
Copy link
Author

chrysn commented Oct 30, 2024

One potentially noteworthy point is the error message when a string that is too long is parsed into a heapless string. That follows the general pattern of having an alloc variant and a non-alloc variant. In theory, given that N is an associated const, this could be string formatted at build time. However, to my understanding that is not possible easily with current stable Rust, and adding a crate as a dependency just to do that string formatting sounds excessive.

@twittner
Copy link
Owner

Thank you for your pull request!

What do you think of creating a crate "minicbor-heapless" that contains wrappers for the various heapless types, and modules with encode/decode functions so that it could be used like #[cbor(with = "minicbor_heapless::string")] for example in minicbor-derive?

I realise that this is a little less convenient compared to having minicbor provide the trait impls as you propose here, however I would like to avoid adding more dependencies to minicbor, optional or not.

@chrysn
Copy link
Author

chrysn commented Oct 31, 2024

I don't think it's quite practical.

Concretely, I'm trying to move users over from serde_cbor (which as we all know is not used any more for good reasons) on to generating more idiomatic CBOR (with numeric keys and all) using minicbor-derive. If they have to jump through extra hoops of finding the right annotations for every other field, that adds a significant amount of complexity.

(Of course there's the alternative of going through minicbor-serde, but that keeps things in the non-CBOR-idiomatic string-keys space -- it does work because heapless imlements serde, but then again serde has around 10x the user base of heapless, and heapless has around 10x the user base of minicbor).

@twittner
Copy link
Owner

twittner commented Nov 3, 2024

Thank your for explaining your motivation. As I mentioned, I do not want to add more dependencies to minicbor and the argument for adding heapless can be made for countless other crates.

@twittner twittner closed this Nov 3, 2024
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