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

Support for C++20 u8string #200

Closed
wants to merge 1 commit into from
Closed

Support for C++20 u8string #200

wants to merge 1 commit into from

Conversation

Apivan
Copy link
Contributor

@Apivan Apivan commented Feb 14, 2023

Since WString already stored utf8_ as std::string. This is just additional interfaces for accepting std::u8string arguments. No conversion needed.

image
https://en.cppreference.com/w/cpp/language/types#char8_t

@emweb
Copy link
Collaborator

emweb commented Feb 27, 2023

Thanks for your contribution.

Ideally Wt would use u8string as its internal representation and use it throughout, but maybe that's more of a Wt 5 thing (we don't have plans for another major release anytime soon).

I have a few remarks:

  • Binary compatibility: if you're using C++20, but Wt was compiled with C++17, this will cause linker errors when the u8string functions are used. I suggest perhaps we make the char8_t/std::u8string functions inline. We do already have an accidental incompatibility due to the date library using std::string_view instead of std::string with C++17, but let's try not to introduce new incompatibilities.
  • We need unit tests for these functions.
  • The operator std::u8string() function is not implemented correctly, consider localized WStrings or WStrings with arguments. You'd have to construct a u8string from the result of toUTF8().
  • The #ifdefs should probably contain the documentation. I think if we want to generate proper documentation, we'll need to check #if defined(__cpp_char8_t) || defined(DOXYGEN_ONLY). Use the doxygen target to verify that the documentation is generated properly.
  • The WString(const std::u8string&) constructor lacks a doc comment.

Regards,
Roel

@Apivan Apivan closed this Mar 9, 2023
@Apivan
Copy link
Contributor Author

Apivan commented Mar 9, 2023

Thank you for your reply. I believe that using std::u8string internally in the future Wt would be a better solution. Initially, I attempted to migrate our workplace's project source code to u8string in order to solve some issues, but I then realized that most third-party libraries do not support this. Changing large project to u8string would be a massive undertaking and cause excessive copying and conversion . So I withdraw this pull request.

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