Skip to content

[libc][z/OS] Remove ASCII trick to fix EBDIC std::from_char #116078

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

Merged
merged 2 commits into from
Nov 13, 2024

Conversation

zibi2
Copy link
Contributor

@zibi2 zibi2 commented Nov 13, 2024

This PR will fix the following lit in all EBCDIC variations on z/OS:
std/utilities/charconv/charconv.from.chars/floating_point.pass.cpp

The trick to test for e and E is working only in ASCII.
The fix is to simply test for both lower and upper case exponent letter e and E respectfully.

@llvmbot
Copy link
Member

llvmbot commented Nov 13, 2024

@llvm/pr-subscribers-libc

Author: Zibi Sarbinowski (zibi2)

Changes

This PR will fix the following lit in all EBCDIC variations on z/OS:
std/utilities/charconv/charconv.from.chars/floating_point.pass.cpp

The trick to test for e and E is working only in ASCII.
The fix is to simply test for both lower and upper case exponent letter e and E respectfully.


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

1 Files Affected:

  • (modified) libc/src/__support/high_precision_decimal.h (+2-1)
diff --git a/libc/src/__support/high_precision_decimal.h b/libc/src/__support/high_precision_decimal.h
index ac11649d1d1686..20088d6d797910 100644
--- a/libc/src/__support/high_precision_decimal.h
+++ b/libc/src/__support/high_precision_decimal.h
@@ -350,7 +350,8 @@ class HighPrecisionDecimal {
     if (!saw_dot)
       this->decimal_point = total_digits;
 
-    if (num_cur < num_len && ((num_string[num_cur] | 32) == 'e')) {
+    if (num_cur < num_len &&
+        (num_string[num_cur] == 'e' || num_string[num_cur] == 'E')) {
       ++num_cur;
       if (isdigit(num_string[num_cur]) || num_string[num_cur] == '+' ||
           num_string[num_cur] == '-') {

Copy link
Contributor

@abhina-sree abhina-sree left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@michaelrj-google michaelrj-google left a comment

Choose a reason for hiding this comment

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

Thanks for catching this, I'll see if there's anywhere else that could use this sort of cleanup.

@zibi2
Copy link
Contributor Author

zibi2 commented Nov 13, 2024

CI failed twice at unrelated generate_test_report.py step. I'm going to merge this now.

@zibi2 zibi2 merged commit e25e886 into llvm:main Nov 13, 2024
5 of 7 checks passed
@nickdesaulniers
Copy link
Member

nickdesaulniers commented Nov 21, 2024

@zibi2 what's a target triple we can use to test EBDIC? --target=s390x-???

@michaelrj-google is making more related changes in #110574.

@perry-ca
Copy link
Contributor

(Zibi's off for a couple days. I'll answer in the meantime)
You don't really need to use a particular target triple. To compile something in EBCDIC you'll need:

  1. the -fexec-charset option (see Create a CharSetConverter class with both iconv and icu support #74516 & Enable fexec-charset option abhina-sree/llvm-project#1) or #pragma convert (follow on to these)
  2. A way to got through the preprocessing paths for defined(__MVS__) && !defined(__NATIVE_ASCII_F). This will then depend on macros or values defined in the z/OS headers (eg. static const mask space = __ISSPACE;)

This would be a specific build of libc++ for EBCDIC. We haven't tried enabling EBCDIC on other platforms. In theory it should work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants