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

z_bytes_decode_into_string and z_bytes_encode_from_string are not symmetrical. #452

Closed
milyin opened this issue Jun 13, 2024 · 4 comments
Closed
Labels
api fix Problem in the API release Part of the next release

Comments

@milyin
Copy link
Contributor

milyin commented Jun 13, 2024

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 #451 about renaming these

@milyin milyin added the release Part of the next release label Jun 13, 2024
@DenisBiryukov91
Copy link
Contributor

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
Copy link
Contributor Author

milyin commented Jun 13, 2024

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
Copy link
Contributor

DenisBiryukov91 commented Jun 13, 2024

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
Copy link
Contributor Author

milyin commented Jun 14, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api fix Problem in the API release Part of the next release
Projects
Status: Done
Development

No branches or pull requests

2 participants