[libcxx-commits] [PATCH] D118851: [SystemZ]:[z/OS]:[libcxx]: fix isascii function to work for z/OS

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__locale:571
 
+    static bool isbasic(char_type __c)
+    {
----------------
SeanP wrote:
> SeanP wrote:
> > 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]).  
> 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.
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.


================
Comment at: libcxx/include/__locale:573
+    {
+#if defined(__MVS__)
+        return (__c >=0 && __c < 256);
----------------
Should this be `#if defined(__MVS__) && !defined(__NATIVE_ASCII_F)` like you did in D118849?


================
Comment at: libcxx/include/__locale:685
+#if defined(__MVS__)
+        return __c ? true : true;
+#else
----------------
Is this intended to always return true, if you please just return `true`. You can use a `(void) __c;` to silence compiler diagnostics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118851