[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:
> > > 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.
> As long as the situation is deterministic, my preference is almost always to //keep// the test coverage. It sounds like in this case you should just add a new `#if`, like
> ```
> #ifdef TEST_COMPILER_MSVC
> bool expect_blank = (????);
> #elif defined(_WIN32)
> bool expect_blank = (9 <= i && i <= 13) || (i == 32);
> #else
> bool expect_blank = (i == '\t' || i == ' ');
> #endif
> ```
> Or, if that wouldn't deterministically pass for some reason (like, the behavior changes between different versions of MSVC and the buildbot can't tell what version it's running, or if we have intel that MSVC is actively working on fixing this), then at least we should `XFAIL: msvc` this whole test, and then if it ever starts passing we could re-investigate. (And by "we" I assume I really mean @CaseyCarter. :))
Ok, sure, that’s doable! And it turns out it ends up less ugly than earlier attempts too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120802