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

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: libcxx/test/std/localization/locale.categories/category.ctype/facet.ctype.special/facet.ctype.char.statics/classic_table.pass.cpp:29-33
+    // Do basic verification of properties; check that specific bits are
+    // set or not set, for some characters, but allow having other bits
+    // set too.
+#define ASSERT_IS_SET(c, bit)  assert((p[static_cast<int>(c)] & (bit)) != 0)
+#define ASSERT_NOT_SET(c, bit) assert((p[static_cast<int>(c)] & (bit)) == 0)
----------------
Quuxplusone wrote:
> This seems like an improvement over the status quo, so if you want to stop here, I'm OK with that.
> But maybe this test should just duplicate all the relevant logic — normally an antipattern, maybe still an antipattern, but...—
> ```
> // in a loop for c from 0 to 255 or whatever
> bool expect_cntrl = (c <= 31) || (127 <= c);
> bool expect_lower = ('a' <= c && c <= 'z');
> bool expect_print = (32 <= c && c <= 126);
> bool expect_punct = (33 <= c && c <= 47) || (58 <= c && c <= 64) || (91 <= c && c <= 96) || (123 <= c && c <= 126);
> bool expect_upper = ('A' <= c && c <= 'Z');
> 
> assert(bool(p[int(c)] & F::alpha) == (expect_upper || expect_lower));
> assert(bool(p[int(c)] & F::lower) == expect_lower);
> assert(bool(p[int(c)] & F::print) == expect_print);
> assert(bool(p[int(c)] & F::punct) == expect_punct);
> ~~~
> ```
> And then for Win32 you just `#if` the `bool expect_blank =` case to something different; and for z/OS eventually, we just `#if` all the `expect_`s to other things.
Ok, I did something like that, please take a look.


================
Comment at: libcxx/test/std/localization/locale.categories/category.ctype/facet.ctype.special/facet.ctype.char.statics/classic_table.pass.cpp:110-112
+    // Intentionally not checking F::blank for '\n'; MS STL has got
+    // F::blank as a mask consisting of 2 bits, and one of them is set
+    // for '\n'. libc++'s F::blank is a single bit which isn't set for '\n'.
----------------
Quuxplusone wrote:
> Does this imply that `std::isblank('\n')` is `true` on Windows? According to cppreference, that's a bug: "In the default C locale, only space (0x20) and horizontal tab (0x09) are classified as blank characters."
No, `::isblank('\n')` does return 0 (MS STL doesn't seem to have a single-argument `std::isblank()`)

In MS STL, `F::blank` is actually a mask consisting of the C runtime level `_BLANK` and `_SPACE`, but the `isblank()` function probably only looks at the `_BLANK` bit. See https://github.com/microsoft/STL/blob/main/stl/inc/xlocale#L2329-L2353.

And for the horizontal tab character, the raw table is missing the blank bit, but the `isblank()` function works around it: https://github.com/microsoft/win32metadata/blob/master/generation/WinSDK/RecompiledIdlHeaders/ucrt/ctype.h#L201


Thus, the high level stuff seems to work pretty reasonably, but the raw `F::classic_table()` table and bitmasks are so-so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120802