[libcxx-commits] [PATCH] D118849: [SystemZ]:[z/OS]:[libcxx]: fix nthLetter issue for charconv header

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__locale:17
 #include <cstdint>
-#include <locale.h>
+#if !defined(_LIBCPP_HAS_NO_LOCALIZATION)
+#  include <locale.h>
----------------
ldionne wrote:
> I don't understand why that's necessary. The intent is for `__locale` not to be includable when `_LIBCPP_HAS_NO_LOCALIZATION` is enabled.
@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.)


================
Comment at: libcxx/include/charconv:474
         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,
----------------
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
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118849/new/

https://reviews.llvm.org/D118849