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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone 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)
----------------
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.


================
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'.
----------------
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."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120802