[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:
> mstorsjo wrote:
> > 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.
> Throughout, please prefer the `(lo <= x && x <= hi)` style I used above.
> https://quuxplusone.github.io/blog/2021/05/05/its-clamping-time/
> Personally I wouldn't do the space-alignment in `expect_punct`, but I guess I don't really care.
> 
> For the `_WIN32` codepath, I'd prefer you put the ifdef around `bool expect_blank =`, like
> ```
> #ifdef _WIN32
> bool expect_blank = (9 <= i && i <= 13) || (i == 32);
> #else
> bool expect_blank = (i == '\t' || i == ' ');
> #endif
> ```
Ok, I can change the formatting of those conditions.

Regarding the blank bit on Windows, I explicitly meant to skip the check entirely for a couple characters, to make the test loose enough that MS STL passes too (your suggestion passes with our code but not with theirs).

On MS STL, the `F::blank` value is set to `_BLANK | _SPACE`, while we have it set to just `_BLANK`. So to cope with that, I’d just skip checking that flag for the lower chars.

I could also skip checking `expect_blank` on Windows altogether, concluding that there’s some mess with that bit. Or just ignore the MS STL case and adjust the expectation for `i == '\t’` which is enough for us and test `expect_blank` for all chars.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120802