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

Rename String::copy_from functions to their respective encodings (parse_latin1, parse_wstring, parse_utf32). #100434

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

Ivorforce
Copy link
Contributor

@Ivorforce Ivorforce commented Dec 15, 2024

The functions are doing interpretation work, not just blind copying, so they should be renamed to reflect what they actually do.

I chose the parse_ prefix to match the existing parse_utf8 and parse_utf16:

godot/core/string/ustring.h

Lines 527 to 531 in b9437c3

Error parse_utf8(const char *p_utf8, int p_len = -1, bool p_skip_cr = false);
static String utf8(const char *p_utf8, int p_len = -1);
Char16String utf16() const;
Error parse_utf16(const char16_t *p_utf16, int p_len = -1, bool p_default_little_endian = true);

Arguably, the interfaces should be made to match and the functions exposed to public, but this would be for another PR. In this one, I'm just trying to clear up semantics a bit, which should be uncontroversial.

@Ivorforce Ivorforce requested a review from a team as a code owner December 15, 2024 15:37
@Ivorforce Ivorforce changed the title Rename String::copy_from to their respective encodings (parse_latin1, parse_wstring, parse_utf32) Rename String::copy_from functions to their respective encodings (parse_latin1, parse_wstring, parse_utf32) Dec 15, 2024
@Ivorforce Ivorforce force-pushed the string-copy-from-rename branch 3 times, most recently from d934814 to 1fc7223 Compare December 15, 2024 18:47
@Ivorforce Ivorforce changed the title Rename String::copy_from functions to their respective encodings (parse_latin1, parse_wstring, parse_utf32) Rename String::copy_from functions to their respective encodings (parse_ascii, parse_wstring, parse_utf32). Fix parse_ascii having undefined behavior above 127. Dec 15, 2024
@Ivorforce Ivorforce force-pushed the string-copy-from-rename branch from 1fc7223 to 7ab35ba Compare December 15, 2024 19:17
@Ivorforce Ivorforce changed the title Rename String::copy_from functions to their respective encodings (parse_ascii, parse_wstring, parse_utf32). Fix parse_ascii having undefined behavior above 127. Rename String::copy_from functions to their respective encodings (parse_latin1, parse_wstring, parse_utf32). Dec 15, 2024
@Ivorforce Ivorforce force-pushed the string-copy-from-rename branch from 7ab35ba to cd3967f Compare December 15, 2024 20:44
@Ivorforce Ivorforce requested a review from a team as a code owner December 15, 2024 20:44
@Ivorforce
Copy link
Contributor Author

MSVC is failing due to the feed_effects bug again (3rd PR where this happens). To quote myself from last time:

To avoid the MSVC compiler issue (which I have previously determined to be a cache issue), I am touching feed_effects.h by inserting an explicit include to a well-known file.
I think it may be caused by the only include of the file being a generated file, possibly leading to the cache resolver running into a race condition when compiling the file. I hope this change can fix the problem for good (though we won't know for sure until future PRs since touching the file already invalidates the cache, fixing the issue).

@Ivorforce Ivorforce force-pushed the string-copy-from-rename branch from cd3967f to 03a8f7d Compare December 15, 2024 20:45
…parse_latin1`, `parse_wstring`, `parse_utf32`).
@Ivorforce Ivorforce force-pushed the string-copy-from-rename branch from 03a8f7d to df3e929 Compare December 15, 2024 20:46
Copy link
Contributor

@kiroxas kiroxas left a comment

Choose a reason for hiding this comment

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

Renaming makes sense.

@akien-mga akien-mga requested a review from bruvzg December 16, 2024 08:29
@@ -33,6 +33,7 @@

#ifdef GLES3_ENABLED

#include "drivers/gles3/shader_gles3.h"
Copy link
Member

Choose a reason for hiding this comment

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

Leftover? How it is related to this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

He explained it in a comment in the PR.

@Chaosus Chaosus added this to the 4.x milestone Dec 16, 2024
@bruvzg
Copy link
Member

bruvzg commented Dec 16, 2024

MSVC is failing due to the feed_effects bug again (3rd PR where this happens). To quote myself from last time

Is it failing locally on a clean build, or only on CI build?

@Ivorforce
Copy link
Contributor Author

Ivorforce commented Dec 16, 2024

MSVC is failing due to the feed_effects bug again (3rd PR where this happens). To quote myself from last time

Is it failing locally on a clean build, or only on CI build?

It's a cache problem; it's not failing for clean builds. Touching the file in any way is enough to wipe the cache and get the builds to pass. I don't know if it can fail locally as well. I've been in talks with the SCons maintainers to get it fixed, but I haven't managed to make an MRP (because I don't use MSVC).

@Chaosus Chaosus modified the milestones: 4.x, 4.4 Dec 18, 2024
@Repiteo Repiteo merged commit 416a86f into godotengine:master Dec 20, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 20, 2024

Thanks!

@Ivorforce Ivorforce deleted the string-copy-from-rename branch December 20, 2024 08:37
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.

5 participants