[SystemZ]:[z/OS]:[libcxx]: fix nthLetter issue for charconv header

[NancyWang2222]

Summary

This patch is to fix issue related to charconv , the problem is in EBCDIC table on z/OS, nth letters are not continuous, they are grouped into a few groups and each group has 9 character. so need new logic in EBCDIC mode to return a correct integer value for nth letters.

[NancyWang2222]

move #include <locale> up one line according to alphabetical order

[NancyWang2222]

only include <locale> when _LIBCPP_HAS_NO_LOCALIZATION is not defined

[NancyWang2222]

put guard in __locale when include locale.h

[abhina.sreeskantharajan]

libcxx/include/charconv

92

I think we need a guard here as well to fix the CI failure.

[NancyWang2222]

need a guard in libcxx/include/charconv as well.

[NancyWang2222]

include <cctype> so tolower function can be recognized when localization is disabled

[NancyWang2222]

ping -:)

[abhina.sreeskantharajan]

LGTM

[Mordante]

LGMT modulo some nits. Please wait for a libc++ approval before committing.

libcxx/include/charconv

433

Since none of the code used in this header is constexpr, it has no effect here.

434

Please use the style already used. I also think the name isn't that clear, maybe __letter_value?

440

I'm not familiar with EBCDIC so I just assume this is correct.

445

Please remove one blank line.

[NancyWang2222]

libcxx/include/charconv

433

Thanks will remove it

434

OK. I will use __letter_value

440

it was tested on z/OS which has both ASCII and EBCDIC

445

will remove blank line. thanks

[NancyWang2222]

address comment: change function name from LetterNum to letter_value, and remove extra blank line , also remove constexpr.

[NancyWang2222]

I rebased with latest master. hope it will fix error.

[NancyWang2222]

match the format suggested by git-clang-format

[ldionne]

libcxx/include/__locale

17 ↗

(On Diff #407889)

I don't understand why that's necessary. The intent is for __locale not to be includable when _LIBCPP_HAS_NO_LOCALIZATION is enabled.

libcxx/include/charconv

433

I'm not sure I understand what that function is actually doing. Is it taking a character and returning an integer that represents the position of that character in a usual human-understandable ordering from a to z? It doesn't handle uppercase characters and that's why you use tolower below, right?

In that case, I think it would make sense to rename it to something like __alphabetical_index or something like that, and it should return a real integer, not the character type taken as an input. In other words, if that's a function from "characters" to "integers", let's be explicit about it -- I suspect it might be useful in other places. We might also want to make it support uppercase letters.

[NancyWang2222]

remove extra space

[NancyWang2222]

libcxx/include/charconv

433

function returns re-ordered index, unlike in ASCIi the letter is continuous, EBCDIC letters are grouped in 3 groups for both lower case and upper case. upper case letter and lower letter in EBCDIC table are different value but similar pattern after order [a-z] and [A-Z]. I will change name to __alphabetical_index . change return type to int.

[NancyWang2222]

libcxx/include/charconv

433

forgot to mention, use tolower function to reduce duplicate code, since both fall into similar patter , just different letter value.

[• Quuxplusone]

libcxx/include/charconv

434

FWIW, I found __letter_value confusing on first reading, because I would think that the "value" of a should be 10 and the "value" of f should be 15. In fact, this function returns 0 for a and 6 for f; i.e., it's returning the letter's zero-indexed index in the alphabet.
It might be too cutesy, but if this were my code, I'd name this function simply __letter_minus_a. It basically returns (__c - 'a'), modulo any relativistic effects due to EBCDIC.

[NancyWang2222]

libcxx/include/charconv

434

@ldionne suggests __alphabetical_index which match what function does. i will use that name. we order letters to be new alphabetical index

[NancyWang2222]

change function name letter_value to alphabetical_index

[NancyWang2222]

@ldionne @Quuxplusone I have addressed comment, Can you help review again. Thanks for the feedback.

[SeanP]

libcxx/include/charconv

440

fyi ... if you want a reference to EBCDIC code points. http://en.wikipedia.org/wiki/Ebcdic. Note EBCDIC has these "variant" code points that change value in different code pages (eg. 1047 vs others). Some characters that change code point are '[', ']', '{', '}', '#' which is just awesome for C/C++ programming.

[NancyWang2222]

ping :)

libcxx/include/charconv

440

Thanks Sean. good information for anyone who wants to know about about EBCDIC.

[NancyWang2222]

Can I have 2nd review from libcxx? Thanks

[NancyWang2222]

please kindly review this patch again. thanks.

[• Quuxplusone]

libcxx/include/__locale

17 ↗

(On Diff #407889)

@NancyWang2222: I don't think this comment from Louis has been addressed/answered. What goes wrong if you remove this diff?
(But I fully expect this will be moot and you can remove this diff, after adopting my suggested rewrite below.)

libcxx/include/charconv

470–471

Is this the only reason you #include <locale> at the top of this file? I don't think we should do that. We should keep the nice fast arithmetic (the old code) for ASCII platforms, at least. Is there any way to express the EBCDIC codepath in speedy terms, or must we call out to tolower when #ifdef __MVS__?
IOW, I suggest lines 474-476 become this instead:

#if defined(__MVS__) && !defined(__NATIVE_ASCII_F)
    if ('a' <= __c && __c <= 'i')
        return {__c - 'a' + 10 < __base, __c - 'a' + 10};
    else if ('j' <= __c && __c <= 'r')
        return {__c - 'j' + 19 < __base, __c - 'j' + 19};
    else if ('s' <= __c && __c <= 'z') 
        return {__c - 's' + 28 < __base, __c - 's' + 28};
    else if ('A' <= __c && __c <= 'I')
        return {__c - 'A' + 10 < __base, __c - 'A' + 10};
    else if ('J' <= __c && __c <= 'R')
        return {__c - 'J' + 19 < __base, __c - 'J' + 19};
    else if ('S' <= __c && __c <= 'Z') 
        return {__c - 'S' + 28 < __base, __c - 'S' + 28};
    else
        return {false, 0};
#else
    else if ('a' <= __c && __c < 'a' + __base - 10)
        return {true, __c - 'a' + 10};
    else
        return {'A' <= __c && __c < 'A' + __base - 10, __c - 'A' + 10};
#endif

libcxx/include/charconv

 #include <__charconv/to_chars_result.h>
 #include <__config>
 #include <__errc>
 #include <cmath> // for log2f
 #include <cstdint>
 #include <cstdlib> // for _LIBCPP_UNREACHABLE
 #include <cstring>
 #include <limits>
+#if !defined(_LIBCPP_HAS_NO_LOCALIZATION)
+#include <locale>
+#endif
 #include <type_traits>
 
 #include <__debug>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
else
             return __r;
         }
     }
 
     return {__r.ptr, errc::result_out_of_range};
 }
 
 template <typename _Tp>
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR _Tp
+__LetterNum(_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 && __LetterNum(__c) < __base - 10, __LetterNum(__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) {