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

BREAKING CHANGE: buffers exposed as binary, as a UTF-8 string #94

Merged
merged 16 commits into from
Mar 27, 2025

Conversation

kafka1991
Copy link
Collaborator

@kafka1991 kafka1991 commented Mar 18, 2025

Overview

This change introduces a minor breaking API change.

In preparation for follow-up work, the ILP buffer will change from always internally serializing to a UTF-8 string to serializing to binary data.

This PR changes the API to allow this, without changing any of the functionality or wire format.

Detail

The APIs that exposes the buffer in Rust (.as_str()), C (line_sender_buffer_peek) and C++ (buffer::peek()) have been changed to return a binary buffer.

In Rust specifically, the .as_str() method has been renamed to .as_bytes().

This PR closes #89

Internally this change swaps Buffer's backing from a String to a Vec<u8>.

@kafka1991 kafka1991 marked this pull request as draft March 18, 2025 10:20
@kafka1991 kafka1991 marked this pull request as ready for review March 18, 2025 14:43
Copy link
Collaborator

@amunra amunra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nearly there!
Some data type inaccuracies to correct and very minor code cleanup required.

@kafka1991
Copy link
Collaborator Author

kafka1991 commented Mar 19, 2025

Nearly there! Some data type inaccuracies to correct and very minor code cleanup required.

Thanks for your review!

@kafka1991 kafka1991 self-assigned this Mar 19, 2025
Copy link
Collaborator

@amunra amunra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more changes needed.

Now that the code has specific support for C++20 the CI tests should compile and run with both C++17 and C++20.

@kafka1991
Copy link
Collaborator Author

A few more changes needed.

Now that the code has specific support for C++20 the CI tests should compile and run with both C++17 and C++20.

Already add c++20 CI tests in pipeline

Copy link
Collaborator

@amunra amunra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more minor changes and one question regading C++14 (which we might have previously supported).

@amunra amunra changed the title change buffer from String to Vec<u8> BREAKING CHANGE: buffers exposed as binary, as a UTF-8 string Mar 21, 2025
@kafka1991
Copy link
Collaborator Author

kafka1991 commented Mar 21, 2025

Some more minor changes and one question regading C++14 (which we might have previously supported).

We didn't support C++14(or lower) since string_view is introduced in c++17

Copy link
Collaborator

@amunra amunra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

@amunra amunra added the DO NOT MERGE This PR should NOT be merged label Mar 24, 2025
@amunra amunra removed the DO NOT MERGE This PR should NOT be merged label Mar 27, 2025
@amunra amunra merged commit 9ce80f2 into main Mar 27, 2025
11 checks passed
@bluestreak01 bluestreak01 deleted the string_buffer_u8 branch March 27, 2025 14:00
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.

Groundwork to prepare for arrays
2 participants