Skip to content

fix: Make xor output Vec<u8> #354

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

Merged
merged 2 commits into from
Aug 23, 2022
Merged

fix: Make xor output Vec<u8> #354

merged 2 commits into from
Aug 23, 2022

Conversation

Ongy
Copy link
Contributor

@Ongy Ongy commented Aug 22, 2022

String is documented as utf8 encoded text.
This can contain multibytes. The cast of char to u8 is lossy.

test_multi_byte fails before the API change.
The other new tests also test common encoding issues.
The don't fail on the old code, but the intermediaries are invalid.

This could also be done with e.g. Box<[u8]>, but String is growable, so Vec<u8> should be fine.

This changes the API, but since the old one necessarily violates documented type properties, IMO it needs to be done.

String is documented as utf8 encoded text.
This can contain multibytes. The cast of char to u8 is lossy.

test_multi_byte fails before the API change.
The other new tests also test common encoding issues.
The don't fail on the old code, but the intermediaries are invalid.
@Ongy Ongy requested review from siriak and imp2002 as code owners August 22, 2022 04:43
Copy link
Member

@siriak siriak left a comment

Choose a reason for hiding this comment

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

The changes look good, but a PR check fails for unrelated reasons. Could you please fix it? Shouldn't take long

Copy link
Member

@siriak siriak left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@siriak siriak merged commit 8ebcc27 into TheAlgorithms:master Aug 23, 2022
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.

2 participants