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

Changed all core::unicode::ustring to std::u32string #13775

Merged

Conversation

calebabutler
Copy link
Contributor

Since IrrlichtMT does not use core::unicode::ustring anywhere except the header in which it is defined (or uses anything else defined in that header anywhere else), we will be able to remove the include/irrUString.h file from the IrrlichtMT repository, removing the last non-STL container from IrrlichtMT.

@sfan5
Copy link
Member

sfan5 commented Sep 3, 2023

Thanks for the contribution. However I was under the impression that we used the custom container implementation because wstring can be UTF-16 or 32 and we require to iterate each codepoint.
thoughts?

@calebabutler
Copy link
Contributor Author

If that's the case, core::unicode::ustring is a bad container for that purpose since it is always UTF-16. A better container is std::u32string. It is my mistake, I thought that Windows was compiling with MinGW which makes wstring UTF-32. I will update the code to use std::u32string.

@calebabutler
Copy link
Contributor Author

Made the change!

@sfan5
Copy link
Member

sfan5 commented Sep 4, 2023

I thought that Windows was compiling with MinGW which makes wstring UTF-32.

We primarily use MinGW, which however still has a 2 byte-sized wchar_t. Even if not we should generally be portable in this regard.

@sfan5 sfan5 self-requested a review September 4, 2023 16:55
@calebabutler
Copy link
Contributor Author

Thanks for correcting me, and it should be portable now.

@calebabutler calebabutler force-pushed the change-core-ustring-to-std-wstring branch from b8db343 to 0518507 Compare September 11, 2023 00:37
I changed all the instances of core::unicode::ustring to std::wstring.
This was not too hard as this data structure is only used in two files.
Since IrrlichtMT does not use core::unicode::ustring anywhere except the
header in which it is defined (or anything else defined in that header),
we will be able to remove the include/irrUString.h file from the
IrrlichtMT repository, removing the last non-STL container from
IrrlichtMT.
Since the Windows build uses UTF-16 for the std::wstring type, I
replaced std::wstring with std::u32string to make the code behave the
same across platforms. Now, in all cases, u32strings are UTF-32, and the
code operates only on valid code points.
@calebabutler calebabutler force-pushed the change-core-ustring-to-std-wstring branch from 0518507 to 1d867e1 Compare September 11, 2023 00:38
src/irrlicht_changes/CGUITTFont.cpp Outdated Show resolved Hide resolved
src/irrlicht_changes/CGUITTFont.cpp Outdated Show resolved Hide resolved
@sfan5 sfan5 added Action / change needed Code still needs changes (PR) / more information requested (Issues) Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements labels Sep 13, 2023
@sfan5 sfan5 changed the title Changed all core::unicode::ustring to std::wstring Changed all core::unicode::ustring to std::u32string Sep 26, 2023
@sfan5 sfan5 removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Sep 26, 2023
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

tested and works on linux & windows

Copy link
Member

@Desour Desour left a comment

Choose a reason for hiding this comment

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

Tested on linux. Colors and translations still seem to work.

<codevct>, std::codecvt_utf8_utf16 (but not stc::codecvt), and std::wstring_convert are deprecated in C++17. But I guess we can change that later.

I'm not entirely sure if the io::path conversions are correct.
I tried setting the font path setting to a path with a non-ascii symbol, and it worked. So it's probably fine.

@sfan5 sfan5 merged commit 3a4bf14 into minetest:master Oct 2, 2023
13 checks passed
Zughy pushed a commit to Zughy/minetest that referenced this pull request Oct 8, 2023
kawogi pushed a commit to kawogi/minetest that referenced this pull request Dec 19, 2023
cosin15 pushed a commit to cosin15/minetest that referenced this pull request Feb 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code quality Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants