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

[libc++][z/OS] Need to define _LIBCPP_HAS_UNICODE to 0 for EBCDIC #119244

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

Conversation

zibi2
Copy link
Contributor

@zibi2 zibi2 commented Dec 9, 2024

This PR is needed since EBCDIC does not support UNICODE.

@zibi2 zibi2 self-assigned this Dec 9, 2024
@zibi2 zibi2 requested a review from a team as a code owner December 9, 2024 18:36
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Dec 9, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 9, 2024

@llvm/pr-subscribers-libcxx

Author: Zibi Sarbinowski (zibi2)

Changes

This PR is needed since EBCDIC does not support UNICODE.


Full diff: https://github.com/llvm/llvm-project/pull/119244.diff

1 Files Affected:

  • (modified) libcxx/include/__config_site.in (+4)
diff --git a/libcxx/include/__config_site.in b/libcxx/include/__config_site.in
index fc01aaf2d8746e..b575a5de8e0d0b 100644
--- a/libcxx/include/__config_site.in
+++ b/libcxx/include/__config_site.in
@@ -28,7 +28,11 @@
 #cmakedefine01 _LIBCPP_HAS_FILESYSTEM
 #cmakedefine01 _LIBCPP_HAS_RANDOM_DEVICE
 #cmakedefine01 _LIBCPP_HAS_LOCALIZATION
+#if defined(__MVS__) && !defined(__NATIVE_ASCII_F)
+#cmakedefine _LIBCPP_HAS_UNICODE 0
+#else
 #cmakedefine01 _LIBCPP_HAS_UNICODE
+#endif
 #cmakedefine01 _LIBCPP_HAS_WIDE_CHARACTERS
 #cmakedefine _LIBCPP_HAS_NO_STD_MODULES
 #cmakedefine01 _LIBCPP_HAS_TIME_ZONE_DATABASE

@zibi2 zibi2 force-pushed the zs_libcxx_has_unicode branch from d387049 to 32c6693 Compare December 9, 2024 18:48
Copy link
Contributor

@perry-ca perry-ca left a comment

Choose a reason for hiding this comment

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

LGTM.

@ldionne
Copy link
Member

ldionne commented Dec 9, 2024

Supporting EBCDIC and z/OS requires a lot more changes than just this unless I am mistaken. I know you folks tried upstreaming a lot of that support a few years ago but AFAICT we never got consensus for moving forward with those changes, which were quite involved. AIX doesn't have the same challenges and we're happy to support it upstream.

I would like to have a broader discussion about EBCDIC support in libc++ before we start cherry-picking small changes like this into our codebase -- that ask is consistent with what we do for other platforms that are not "officially supported".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants