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

Add String::ascii creator functions, to parse a char buffer as ASCII. #101304

Merged
merged 1 commit into from
Mar 9, 2025

Conversation

Ivorforce
Copy link
Member

@Ivorforce Ivorforce commented Jan 8, 2025

The function will log errors if any characters above value 127 are found. This is most similar to parse_latin1, but parse_latin1 will silently and incorrectly parse values above 127 as 'extended latin set' characters, when ASCII only is expected. This would be especially incorrect if the input is utf8, because the string will silently have incorrect length and misinterpreted characters.

As discussed in #101293 (comment), two functions in plist are known to expect ASCII-only strings, so they are used as an excuse to call the new function. It would be correct to call parse_ascii on many other occasions (see #100641), but I do not have the broad knowledge to search out all these instances. Instead, the function is added here such that future contributions can call it to safely parse ASCII when it is expected.

Requires and includes #101293 (because of implicit conversions).

@Ivorforce Ivorforce requested a review from a team as a code owner January 8, 2025 18:43
@Ivorforce Ivorforce force-pushed the string-parse-ascii branch 2 times, most recently from 21364f4 to 0d288be Compare January 8, 2025 19:50
@Ivorforce Ivorforce requested review from a team as code owners January 8, 2025 19:50
@Ivorforce
Copy link
Member Author

Tests were failing because of implicit conversions, but it should work fine on top of #101293. I have rebased the PR accordingly.

Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

It's definitely cleaner to have dedicated functions for these.

@Repiteo
Copy link
Contributor

Repiteo commented Mar 7, 2025

#101293 has been merged, this is ready for a rebase!

The function will log errors if any characters above value 127 are found.
@Ivorforce Ivorforce force-pushed the string-parse-ascii branch from 5022418 to b6cfcde Compare March 7, 2025 23:01
@Ivorforce
Copy link
Member Author

Rebase complete! PR is looking much cleaner now =)

@Repiteo Repiteo merged commit 3a0b8da into godotengine:master Mar 9, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Mar 9, 2025

Thanks!

@Ivorforce Ivorforce deleted the string-parse-ascii branch March 9, 2025 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants