Add initial implementation of Postcard-C#220
Conversation
✅ Deploy Preview for cute-starship-2d9c9b canceled.
|
i509VCB
left a comment
There was a problem hiding this comment.
I also noticed that encoding u128 and i128 is not currently possible. Although I do understand that 128-bit types are special and require checking the compiler with proprocessor code.
| postcard_error_t postcard_encode_byte_array(postcard_slice_t* slice, | ||
| const uint8_t* bytes, | ||
| size_t length); |
There was a problem hiding this comment.
Would it be helpful to have a way to determine if there is enough room in the slice to push the array, without actually writing anything? (And other functions like this)
There was a problem hiding this comment.
This is a potential problem across all of the functions, not just byte array. The trouble is that it would make encoding essentially a two-step operation: one to check how much size a varint takes, and another to actually encode it.
In the use cases I envisioned for this, I don't think this will cause practical problems. The first is a higher-level code generator that will pre-allocate memory based on the calculated encoded size. Another is serializing messages to fixed-size buffers. In that case, I'm not too concerned if there is half-written data, as long as the functions properly error out.
I think we can recommend that users run the postcard_size functions themselves if they never want to write data when the buffer isn't big enough.
There was a problem hiding this comment.
a bit tangential to the discussion, but relevant for this (and similar APIs):
rather than passing a pointer you could pass an array - there's a fancy C notation (since C99) which allows you to specify the parameter which gives the length and the compiler will validate it. but the length parameter has to come before the array, otherwise it doesn't work (until this proposal gets standardised).
this works:
int foo(unsigned int len, char foo[static len]);| @@ -0,0 +1,121 @@ | |||
| #include "postcard.h" | |||
There was a problem hiding this comment.
There should probably be an SPDX header here. I do plan to use Postcard-C in projects where this is required.
There was a problem hiding this comment.
Yeah that makes sense. I can add a SPDX header for MIT + Apache dual license to match the rest of the repo
| typedef enum { | ||
| POSTCARD_SUCCESS = 0, | ||
| POSTCARD_ERROR_BUFFER_TOO_SMALL, | ||
| POSTCARD_ERROR_INVALID_INPUT, | ||
| POSTCARD_ERROR_INCOMPLETE_DATA, | ||
| POSTCARD_ERROR_OVERFLOW | ||
| } postcard_error_t; |
There was a problem hiding this comment.
I would prefer each error code to get an explicitly defined value. Whether the error values are positive or negative values is a different question.
| /// - string - a pointer to the string to encode. This string should be utf8 | ||
| /// encoded string. It shouldn't be null-terminated. |
There was a problem hiding this comment.
Is should a strong enough guarantee here? Or should it be considered undefined behavior if the string is NOT valid UTF-8?
| /// - slice - a pointer to an initialized `postcard_slice_t` | ||
| /// - string - a pointer to the string to encode. This string should be utf8 | ||
| /// encoded string. It shouldn't be null-terminated. | ||
| /// - length - the length of the string |
There was a problem hiding this comment.
To make it more clear that this is talking about each byte to be written and not the number of codepoints.
| /// - length - the length of the string | |
| /// - length - the length of the string in bytes |
| @@ -0,0 +1,1184 @@ | |||
| #ifndef POSTCARD_H | |||
There was a problem hiding this comment.
Top of header documentation should probably include a link to the specification.
| return POSTCARD_ERROR_BUFFER_TOO_SMALL; | ||
|
|
||
| uint32_t bits; | ||
| memcpy(&bits, &value, sizeof(bits)); |
There was a problem hiding this comment.
Should we use the memcpy in the standard library or make the caller pick the memcpy?
One pattern I often see is that a library has a seperate memcpy function, like a postcard_memcpy. This would let the caller define it's own memcpy implementation.
There was a problem hiding this comment.
I've been going back and forth on what the best behavior is here. Personally I like to use a minimal libc (i.e newlib) on embedded devices, but I can understand not wanting to rely on that. I think the ergonomic benefits for non-embedded development are important though.
A good way to do it might be to use __has_include to check if string is available and use that version of memcpy if it exists. Alternatively we could use a preprocessor definition to allow the user to optionally override the memcpy impl
| } postcard_slice_t; | ||
|
|
||
| /// postcard_c error codes | ||
| typedef enum { |
There was a problem hiding this comment.
i know that this is currently pure C, but doesn't pulling it into a C++ code base pollute the namespace (unless you #include it within a namespace)?
would it make sense to make the header C & C++ and use #ifdef __cplusplus copiously to decide between the two? that'd e.g. allow using enum class rather than typedef enum
This PR adds a single header file implementation of the postcard spec in C. It's designed to be a relatively low-level API that allows users to manually write serialization and deserialization logic. It uses a BYOB (bring your own buffer) design, where the user has to pass a pre-allocated buffer of data. As such, it does not do any memory management on its own.
You can find more details on usage in
README.mdandexample.cI've tested this implementation as part of a C++ code generation system, and it works well on the types I've thrown at it. That being said, I can't guarantee that it is bug-free, and I would like to add a test suite that tests this implementation against the Rust implementation.
I will also be open-sourcing a C++ code generator that uses postcard-schema and postcard-c to generate high-level C++ structs.