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

[Sean via Phabricator via libcxx-commits]

SeanP added inline comments.


================
Comment at: libcxx/src/locale.cpp:1029
+      *low =
+          __is_valid_in_encoding(*low) ? static_cast<char>(__classic_upper_table()[static_cast<size_t>(*low)]) : *low;
 #else
----------------
Quuxplusone wrote:
> Changes like this one make me nervous. The original purpose of `isascii(x)` seems to have been a bounds check: it says "yes, `x` is in the range 0 to 127 inclusive." That doesn't really have anything to do with the ASCII versus EBCDIC question — hence the mass renaming to `__is_valid_in_encoding`, on which I'm neutral. However, it is //very important// to all of these table lookups: If `x` isn't in the range 0 to 127, e.g. if x is `-1` or `10000`, then these lookups will have UB.
> 
> Can you explain why you want to make this change? Specifically, what behaves wrongly on your platform if we //don't do any of this?//
The tables on z/OS have the range 0-255.  This is true for both ASCII and EBCDIC.  The tables need to be 0-255 since EBCDIC characters are spread over this entire range (eg. '9' is 0xf9).  The system uses the same range in for the ASCII tables which is why the code uses 0-255 for z/OS.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118851