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

Core: Integrate CharStringT #98439

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Oct 22, 2024

Taking heavy inspiration from the above PR, this aims to consolidate CharString and Char16String with a common template core: CharStringT. Not only does this have the benefit of code uniformity, but it also gives us access to new character string classes: CharWideString & Char32String (Also Char8String when char8_t is eventually supported). The only major changes from the godot-cpp implementation were making some functions inlined where possible & adding equality operators.

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks! This looks great to me

It very nearly matches the godot-cpp version, but with only minor changes (mostly const -> constexpr and inlining some stuff) that all make sense to me. I guess we'll need to copy those differences back to godot-cpp once this is merged :-)

core/string/ustring.cpp Outdated Show resolved Hide resolved
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.

Replacing CharString and Char16String with a template seems fine.

But there's no reason for wchar_t (ambiguous type) and char32_t versions, char8_t also completely useless by its own (can be replacement for char8_t, but what the point of doing so).

@Repiteo
Copy link
Contributor Author

Repiteo commented Oct 31, 2024

I'm not entirely certain of the original intent behind the wchar_t/char32_t strings (maybe @dsnopek can clarify), this was more about granting parity. If they're truly redundant, then I suppose they can be discarded. I guess char8_t could have a niche if char was reworked to be specific to ASCII, but that'd involve quite the refactor

@bruvzg
Copy link
Member

bruvzg commented Oct 31, 2024

I guess char8_t could have a niche if char was reworked to be specific to ASCII, but that'd involve quite the refactor

Why do we want two different type that are exactly same, unless char8_t version have some UTF-8 specific methods. Why would it, it's just a wrapper to avoid manual memory management?

I'm not entirely certain of the original intent behind the wchar_t/char32_t strings (maybe @dsnopek can clarify), this was more about granting parity.

Not sure why these exist in the godot-cpp either, I guess it was added since there are string_new_with_wide_chars, string_new_with_utf32_chars, string_to_utf32_chars, and string_to_wide_chars low level String methods (which were intended for use with buffers provided by external API).

@Repiteo
Copy link
Contributor Author

Repiteo commented Nov 1, 2024

Why do we want two different type that are exactly same

I would say for similar reasons that we use char16_t/char32_t instead of uint16_t/uint32_t respectively, but we already have char so that's a harder sell. I suppose char8_t is explicitly unsigned, making it distinct from both char having implementation-defined signedness & unsigned char which is just uint8_t. Still, probably not the biggest selling point, so it's likely that anything integrating the type would be either part of a wider String refactor or extremely barebones

@dsnopek
Copy link
Contributor

dsnopek commented Nov 1, 2024

I'm not entirely certain of the original intent behind the wchar_t/char32_t strings (maybe @dsnopek can clarify), this was more about granting parity.

I also don't really know the intention of CharWideString and Char32String either. They were added to godot-cpp in PR godotengine/godot-cpp#602 (which is the original GDNative -> GDExtension PR). Then I just kept them when I did the conversion to a template in PR godotengine/godot-cpp#1150.

Not sure why these exist in the godot-cpp either, I guess it was added since there are string_new_with_wide_chars, string_new_with_utf32_chars, string_to_utf32_chars, and string_to_wide_chars low level String methods (which were intended for use with buffers provided by external API).

Could be.

They end up creating an API discrepancy between Godot and godot-cpp, which isn't great. We probably should consider either (1) removing them from godot-cpp, or (2) adding them to Godot, so the APIs align. If we really do want to have them in godot-cpp for use with external APIs, they wouldn't cause any harm being in Godot as well. Adding them to Godot would probably mean also adding String::utf32() and String::wide_string().

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.

3 participants