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

[Nancy Wang via Phabricator via libcxx-commits]

NancyWang2222 added inline comments.


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


================
Comment at: libcxx/include/__locale:685
+#if defined(__MVS__)
+        return __c ? true : true;
+#else
----------------
Mordante wrote:
> Is this intended to always return true, if you please just return `true`. You can use a `(void) __c;` to silence compiler diagnostics.
clang will emit error if argument __c is not used in function. that is why I checked __c. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118851