Add support for multiple heapless and heapless-bytes versions
#13
Conversation
sosthene-nitrokey
force-pushed
the
serialize-heapless
branch
from
20421a4
to
8ce6e24
Compare
sosthene-nitrokey
force-pushed
the
serialize-heapless
branch
2 times, most recently
from
38aca7c
to
f117b49
Compare
| } | ||
|
|
||
| #[cfg(feature = "heapless-bytes-v0-3")] | ||
| /// Append serialization of object to existing bytes, returning length of serialized object. | ||
| pub fn cbor_serialize_extending_bytes<T: ?Sized + serde::Serialize, const N: usize>( |
There was a problem hiding this comment.
Should we deprecate this function in favor of cbor_serialize_to? It would mean that we could need additional type annotations in some cases, but it is somewhat inconsistent to only provide this function for heapless-bytes 0.3.
We don’t need to do it in this PR but should at least create an issue for it if we don’t.
There was a problem hiding this comment.
It was left for backwards compatibility for Nitrokey/nitrokey-3-firmware#538, otherwise I would have removed it yes.
There was a problem hiding this comment.
I was just thinking of adding the #[deprecated] attribute. We can then remove it later.
There was a problem hiding this comment.
I think I would:
- merge this as is (so we don't get warnings in our current releases)
- remove it for the 0.5.0 release.
There was a problem hiding this comment.
If you want a compatible intermediate state, we should also add heapless-bytes-v0-3 to the default features (and remove it for the release).
|
|
||
| object.serialize(&mut ser)?; | ||
|
|
||
| Ok(ser.into_inner().len() - len_before) | ||
| } | ||
|
|
||
| #[cfg(feature = "heapless-bytes-v0-3")] | ||
| /// Serialize object into newly allocated Bytes. | ||
| pub fn cbor_serialize_bytes<T: ?Sized + serde::Serialize, const N: usize>( |
There was a problem hiding this comment.
Same as above – a replacement could use Writer + Default.
There was a problem hiding this comment.
Same as above, kept for backwards compatibility. But maybe we tag this as a nitrokey release, and merge it removing the old function (maybe in a separate PR, that cleans everything up for 0.5.0).
src/ser.rs
Outdated
| // #[derive(Debug)] | ||
| // pub struct SliceWriter<'a> { | ||
| // slice: &'a mut [u8], | ||
| // index: usize, | ||
| // } |
There was a problem hiding this comment.
Why do you want to keep this?
| self.written += buf.len(); | ||
| self.writer.write_all(buf) |
There was a problem hiding this comment.
I think written should only be increased if write_all is successful. I don’t think this is the case today (or maybe ever) but theoretically, serialization could continue after a failure.
There was a problem hiding this comment.
If we follow the write_all from the standard library we could also have a write_all failure that has written some bytes. This is also internal API, and can be adapted in the future if required.
If we want to think forward, we should probably have a only a write function, and have write_all be a default impl.
There was a problem hiding this comment.
While the API is internal, the behavior of write_all is part of the public API via Serializer.
We could also just use core_io instead. |
|
I think it would not work for |
|
|
|
Yeah, I just found it too. For |
sosthene-nitrokey
force-pushed
the
serialize-heapless
branch
from
f117b49
to
c1aa131
Compare
This allows better backwards compatibility and serializing to heapless directly. Given #11 I went with a breaking change update (the `Serializer` field was public and existing implementations are put behind a breaking change)
sosthene-nitrokey
force-pushed
the
serialize-heapless
branch
from
713928f
to
32f85c5
Compare
Depends on