eclipse-zenoh / zenoh-c

C API for Zenoh
http://zenoh.io
Other
70 stars 52 forks source link

z_bytes_decode_into_string and z_bytes_encode_from_string are not symmetrical. #452

Closed milyin closed 2 days ago

milyin commented 3 weeks ago

Describe the release item

z_bytes_encode_from_string requires null-terminated string, while z_bytes_decode_into_string don't add null to the end. In fact, the pair function for z_bytes_encode_from_string is z_bytes_decode_from_sllice. I'd propose to return previous behaviour and make z_bytes_encode_from_string take the loan of z_owned_string_t. And rename existing z_bytes_encode_from_string to something which makes clear, that the parameter is null-terminated

Rename also z_bytes_serialize_from_silce to ..._from_buffer as it actually accepts buffer and length, not our slice object. If needed, we can later add ...from_slice which accepts our slice.

See also https://github.com/eclipse-zenoh/zenoh-c/issues/451 about renaming these

DenisBiryukov91 commented 3 weeks ago

Same holds for slice too. But this was done on purpose to reduce amount of boilerplate code (and to stress that no extra allocation is required). Technically I would rather prefer to allow decoding into user provided buffer, with specified length (although similar behavior can already be achieved using z_bytes_reader).

milyin commented 3 weeks ago

I think this doesn't stress the feature of "no-alllocation" enough. At least for me the existing naming doesn't make clear that the allocation doesn't happen. And btw, what do you mean by "no extra allocation". Does this mean that now z_bytes_encode_from_string just stores pointer to external string?

DenisBiryukov91 commented 3 weeks ago

yes, this function just stores pointer (there is a _copy variant that makes a copy), but I meant no allocation in the sense that user does not have to create z_owned_string_t explicitly if he has const char (although he can always create z_view_string_t, which does not allocate - but this is more code to write).

milyin commented 3 weeks ago

I think the primary thing to do is to rename z_bytes_encode_from_string (and the _copy variant) to something like z_bytes_serialize_from_str (this can be combined with #451). Adding function z_bytes_serialize_from_string which accepts z_owned_string_t makes sense I think, but it's not absolutely necessary. Also @DenisBiryukov91 proposed to add more serializing functions which writes to user's raw buffer. @DenisBiryukov91, can you create task for it?