[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 a comment.

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.



================
Comment at: libcxx/include/__locale:571
 
+    static bool isbasic(char_type __c)
+    {
----------------
NancyWang2222 wrote:
> Mordante wrote:
> > 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.
> I will use new function name. 
@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.


================
Comment at: libcxx/src/locale.cpp:1026
 #elif defined(__NetBSD__)
         *low = static_cast<char>(__classic_upper_table()[static_cast<unsigned char>(*low)]);
 #elif defined(__GLIBC__) || defined(__EMSCRIPTEN__) || defined(__MVS__)
----------------
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.


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

https://reviews.llvm.org/D118851