[SystemZ]:[z/OS]:[libcxx]: fix isascii function to work for z/OS

[NancyWang2222]

Summary

current logic in isascii function assumes character is ASCII. it is true when character is between 0 - 255. we need to change logic so it will work on both ASCII and EBCDIC mode.

[muiez]

LGTM

[SeanP]

libcxx/include/__locale

573

Rather than making this a static member function, I suggest creating an anonymous namespace outside the class and putting the function in there and as an inline function. You can then get rid of the duplicate definition in the other class.

I suspect to that for MVS we need to have one check for EBCDIC ([0,255]) and another for ASCII ([0,127]).

[SeanP]

libcxx/include/__locale

573

Nancy reminded me off-line that the char traits tables for MVS work on the range 0-255 for both ASCII and EBCDIC mode on MVS. The code, as is, is good.

[NancyWang2222]

ping -:)

[Mordante]

libcxx/include/__locale

573

Names in the library should be uglified, so for example __isbasic. I also like a better name, isascii is quite clear, isbasic not really. How about __is_valid_in_encoding? Maybe a bit long, but it's clear what you test.

575

Should this be #if defined(__MVS__) && !defined(__NATIVE_ASCII_F) like you did in D118849?

678

Is this intended to always return true, if you please just return true. You can use a (void) __c; to silence compiler diagnostics.

[SeanP]

libcxx/include/__locale

575

Thanks for thinking about ebcdic. The purpose of this function is to make sure the value in the range supported by the __tab indexing. On z/OS, the __tab is 256 chars regardless of ascii or ebcdic so the 0-255 range check works for both ascii & ebcdic.

I like the proposed new name.

[NancyWang2222]

libcxx/include/__locale

573

I will use new function name.

678

clang will emit error if argument c is not used in function. that is why I checked c.

[NancyWang2222]

libcxx/include/__locale

678

I will add void(__c);

[NancyWang2222]

change isbasic function name and fix return __c ? true: true statement

[NancyWang2222]

fix function name for locale.cpp as well.

[NancyWang2222]

run git-clang-format for the changes

[Mordante]

In general it looks good, some minor nits. I'd like to see it pass the CI before approving. I expect the ASAN build to fail since a recent compiler update in the build-nodes causes failures.

libcxx/include/__locale

573

@NancyWang2222 when you addressed a comment can you mark it as done? That makes it easier to see which comments haven't been addressed yet.

libcxx/src/locale.cpp

1026 ↗

(On Diff #407552)

I don't mind the formatting in the other #if blocks, but then please update the indention here also.
Please also look at the other indention changes you made.

[NancyWang2222]

libcxx/src/locale.cpp

1026 ↗

(On Diff #407552)

sure . I will update it.

[NancyWang2222]

fix indentation for the function modified.

[Mordante]

LGTM, please wait for a libc++ approval before committing.

[ldionne]

libcxx/include/__locale

573

Here and below.

678

Why is this intended to always return true?

[NancyWang2222]

libcxx/include/__locale

573

i will add.

678

isascii function is not right test for z/OS, char type is between [0-255] which is valid character indexing in __tab function on z/OS for both ASCII and EBCDIC.

[NancyWang2222]

add _LIBCPP_HIDE_FROM_ABI for function __is_valid_in_encoding

[NancyWang2222]

change function name and return type

[NancyWang2222]

please ignore comment.

[NancyWang2222]

@ldionne hi Louis, Can you help review again? Thanks everyone for providing feedback.

[NancyWang2222]

kindly ping for review or approval from libcxx group

[NancyWang2222]

can I have 2nd review from libcxx?

[• Quuxplusone]

libcxx/src/locale.cpp

1029 ↗

(On Diff #408051)

Changes like this one make me nervous. The original purpose of isascii(x) seems to have been a bounds check: it says "yes, x is in the range 0 to 127 inclusive." That doesn't really have anything to do with the ASCII versus EBCDIC question — hence the mass renaming to __is_valid_in_encoding, on which I'm neutral. However, it is very important to all of these table lookups: If x isn't in the range 0 to 127, e.g. if x is -1 or 10000, then these lookups will have UB.

Can you explain why you want to make this change? Specifically, what behaves wrongly on your platform if we don't do any of this?

[SeanP]

libcxx/src/locale.cpp

1029 ↗

(On Diff #408051)

The tables on z/OS have the range 0-255. This is true for both ASCII and EBCDIC. The tables need to be 0-255 since EBCDIC characters are spread over this entire range (eg. '9' is 0xf9). The system uses the same range in for the ASCII tables which is why the code uses 0-255 for z/OS.

[• Quuxplusone]

libcxx/include/__locale

678

IIUC now (based on @SeanP's comment below), __is_valid_in_encoding(x) means no more or less than "0 <= x && x < N, where N is the number of elements in the array that was passed to the constructor on line 681." For most platforms this N is 128; for z/OS in particular, this N is 256.

Is this N always the same as ctype::table_size from lines 777–779? What goes wrong if we make this function simply

static bool __is_in_range(size_t __c) { return __c < table_size; }

? See
https://en.cppreference.com/w/cpp/locale/ctype_char/classic_table
https://en.cppreference.com/w/cpp/locale/ctype_char/table

Personally (although @ldionne is welcome to overrule me) I'd like to move away from the existing code's use of isascii(x) as a shorthand for x < 128, because this bounds check doesn't really have anything to do with ASCII, and it's silly to have to read a man page just to find out it means x < 128. If we mean x < table_size, we should just say that.

libcxx/src/locale.cpp

1029 ↗

(On Diff #408051)

Ah, thanks. I've resumed the comment thread on __is_valid_in_encoding above.

[SeanP]

libcxx/include/__locale

678

Good observation. However, I noticed that table_size==256. I don't think that we can assume that ctype<char>::classic_table and __tab_ have the same range. The code as it is will return false for anything out of range of __tab_ for ctype<char>::classic_table. If they should have the same size then we would have to update table_size for non-z/OS platforms. It is currently set to 256 for all platforms. I worry this will break people.

If you think the __is_in_range() fucntion should return table_size, I'd rather do that in a separate patch to avoid mixing it in with making things work for z/OS.

[NancyWang2222]

ping for the review. Thanks

[• Quuxplusone]

libcxx/include/__locale

678

My current thought is that there are at least two, maybe three, changes going on in this PR:
(1) Rename isascii(x) to __is_valid_in_encoding(x). I agree that isascii is the wrong name, but I still hypothesize that the right name is nothing specifically to do with "encoding," but more like __is_valid_table_index. Sometimes the table in question is __tab_; sometimes it's classic_table(); do we want two different functions depending on which table we're talking about? Are all these tables guaranteed to have the same size? IMHO this renaming should be its own PR.
(2) Specifically for z/OS, change the behavior of __is_valid_in_encoding (or whatever its name(s) ends up as). This seems uncontroversial to me, but sadly is blocked on (1).
(3?) Other ad-hoc EBCDICifications, like replacing uses of x - 'a'? I see some non-functional diffs involving L'A' - L'a' and so on, but I'm not sure if there might be some functional diffs hiding among them. Anyway, I would imagine these would be uncontroversial, if they exist.

libcxx/include/__locale

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP___LOCALE
 #define _LIBCPP___LOCALE
 
 #include <__availability>
 #include <__config>
 #include <cctype>
 #include <cstdint>
-#include <locale.h>
+#if !defined(_LIBCPP_HAS_NO_LOCALIZATION)
+#  include <locale.h>
+#endif
 #include <memory>
 #include <mutex>
 #include <string>
 #include <utility>
 
 #if defined(_LIBCPP_MSVCRT_LIKE)
 # include <cstring>
 # include <__support/win32/locale_win32.h>

 {
 public:
     typedef wchar_t char_type;
 
     _LIBCPP_INLINE_VISIBILITY
     explicit ctype(size_t __refs = 0)
         : locale::facet(__refs) {}
 
     _LIBCPP_INLINE_VISIBILITY
     bool is(mask __m, char_type __c) const
     {
         return do_is(__m, __c);
     }
 
     _LIBCPP_INLINE_VISIBILITY
     const char_type* is(const char_type* __low, const char_type* __high, mask* __vec) const
     {
         return do_is(__low, __high, __vec);
     }

 public:
     typedef char char_type;
 
     explicit ctype(const mask* __tab = nullptr, bool __del = false, size_t __refs = 0);
 
     _LIBCPP_INLINE_VISIBILITY
     bool is(mask __m, char_type __c) const
     {
         return isascii(__c) ? (__tab_[static_cast<int>(__c)] & __m) !=0 : false;
     }
 
     _LIBCPP_INLINE_VISIBILITY
     const char_type* is(const char_type* __low, const char_type* __high, mask* __vec) const
     {
         for (; __low != __high; ++__low, ++__vec)
             *__vec = isascii(*__low) ? __tab_[static_cast<int>(*__low)] : 0;
         return __low;

libcxx/include/charconv

 #include <cstdint>
 #include <cstdlib> // for _LIBCPP_UNREACHABLE
 #include <cstring>
 #include <limits>
 #include <type_traits>
 
 #include <__debug>
 
+#if defined(_LIBCPP_HAS_NO_LOCALIZATION)
+#  include <cctype>
+#else
+#  include <locale>
+#endif
+
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 _LIBCPP_PUSH_MACROS
 #include <__undef_macros>
 
 _LIBCPP_BEGIN_NAMESPACE_STD
else
             return __r;
         }
     }
 
     return {__r.ptr, errc::result_out_of_range};
 }
 
 template <typename _Tp>
+inline _LIBCPP_INLINE_VISIBILITY int __alphabetical_index(_Tp __c) {
+#  if defined(__MVS__) && !defined(__NATIVE_ASCII_F)
+  if ('a' <= __c && __c <= 'i')
+    return __c - 'a';
+  else if ('j' <= __c && __c <= 'r')
+    return __c - 'j' + 9;
+  else if ('s' <= __c && __c <= 'z') 
+    return __c - 's' + 18;
+  else
+    return 27;
+#  else
+    return __c - 'a';
+#  endif
+ }
+
+template <typename _Tp>
 inline _LIBCPP_INLINE_VISIBILITY bool
 __in_pattern(_Tp __c)
 {
     return '0' <= __c && __c <= '9';
 }
 
 struct _LIBCPP_HIDDEN __in_pattern_result
 {
     bool __ok;
     int __val;
 
     explicit _LIBCPP_INLINE_VISIBILITY operator bool() const { return __ok; }
 };
 
 template <typename _Tp>
 inline _LIBCPP_INLINE_VISIBILITY __in_pattern_result
 __in_pattern(_Tp __c, int __base)
 {
     if (__base <= 10)
         return {'0' <= __c && __c < '0' + __base, __c - '0'};
     else if (__in_pattern(__c))
         return {true, __c - '0'};
-    else if ('a' <= __c && __c < 'a' + __base - 10)
-        return {true, __c - 'a' + 10};
-    else
-        return {'A' <= __c && __c < 'A' + __base - 10, __c - 'A' + 10};
+    __c = _VSTD::tolower(__c);
+    return {'a' <= __c && __alphabetical_index(__c) < __base - 10,
+            __alphabetical_index(__c) + 10};
 }
 
 template <typename _It, typename _Tp, typename _Fn, typename... _Ts>
 inline _LIBCPP_INLINE_VISIBILITY from_chars_result
 __subject_seq_combinator(_It __first, _It __last, _Tp& __value, _Fn __f,
                          _Ts... __args)
 {
     auto __find_non_zero = [](_It __first, _It __last) {