[libcxx-commits] [PATCH] D120802: [libcxx] [test] Fix the classic_table test on Windows

Tue Mar 8 10:54:31 PST 2022

Mordante accepted this revision.
Mordante added a comment.
This revision is now accepted and ready to land.

In D120802#3354114 <https://reviews.llvm.org/D120802#3354114>, @mstorsjo wrote:

> In D120802#3354110 <https://reviews.llvm.org/D120802#3354110>, @Quuxplusone wrote:
>
>> IMHO this test starts out really bad, but these changes don't improve it in the right direction. I'd suggest either
>> (1) leave it XFAIL'ed for Windows with no FIXME, or
>> (2) completely rewrite it
>> (3) What about z/OS and EBCDIC? I bet @NancyWang2222 and @SeanP are eventually going to get around to this test, and then it's going to have to undergo surgery again. So we should plan ahead for that, too.
>
> Yes, this isn't really pretty... FWIW, MS STL also has slightly different values for all these (and has a slightly different literal values for C++ ctype constants too). So this test right now tests way, way more than what the C++ standard would imply, I think.
>
> So should we reduce the test down to first just minimal testing based on what the standard implies (call the function to make sure it doesn't crash, then check that e.g. uppercase/lowercase ASCII 'A' is flagged correctly - although I guess we can't assume that either, for EBCDIC), and then for specific platforms and/or C++ library implementations, do the full check like we do here?

I'm not even sure the test will pass at all at EBCDIC at all, with the ranges used. Specifically `expect_punct` seems to expect ASCII. However I think when we want to support EBCDIC we either need a large `#if` or create a separate test for EBCDIC. At the moment we don't have a CI to test EBCDIC. I don't see that as a blocker for this patch.

LGTM!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120802