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

Resources: correct string encoding conversion #128

Merged
merged 1 commit into from
Dec 7, 2023
Merged

Conversation

compnerd
Copy link
Contributor

@compnerd compnerd commented Dec 7, 2023

The UTF8 to UTF16 conversion was error-prone as it truncated the top-bit and used the wrong value for the character count for the string creation. This uses Foundation to perform the string conversion and counts the new character length.

The string conversion of the character to UTF16 also houses another failure point by not ensuring that we do not require a surrogate pair for the character encoding. However, leave that issue in place but remove the use of the withWideChars conversion. In the process we avoid an allocation of a String by directly using the UTF16 view and stripping the first integral value (potentially half of a surrogate pair).

The UTF8 to UTF16 conversion was error-prone as it truncated the top-bit
and used the wrong value for the character count for the string
creation.  This uses Foundation to perform the string conversion and
counts the new character length.

The string conversion of the character to UTF16 also houses another
failure point by not ensuring that we do not require a surrogate pair
for the character encoding.  However, leave that issue in place but
remove the use of the `withWideChars` conversion.  In the process we
avoid an allocation of a `String` by directly using the UTF16 view and
stripping the first integral value (potentially half of a surrogate
pair).
@compnerd compnerd requested a review from a team as a code owner December 7, 2023 23:22
Copy link
Collaborator

@stevenbrix stevenbrix left a comment

Choose a reason for hiding this comment

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

thank you!

Copy link

@darinf darinf left a comment

Choose a reason for hiding this comment

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

LGTM, confirmed this fixes the bug.

@compnerd compnerd merged commit b261b11 into main Dec 7, 2023
1 check passed
@compnerd compnerd deleted the compnerd/conversion branch December 7, 2023 23:36
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.

3 participants