[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:75-94
+#ifndef _WIN32
+        // Check that exactly the expected bits are set and nothing else.
+        // This only works if all these constants are invidual separate
+        // bits, not masks aggregated from multiple bits.
+        mask set = 0;
 
+        if (expect_cntrl) set |= F::cntrl;
----------------
mstorsjo wrote:
> Quuxplusone wrote:
> > I thought the goal was still to get rid of this big `#ifndef` block. And I think you //can// get rid of line 92 — it's now redundant with lines 64–73, right?
> > 
> > Would it be correct on all platforms to replace line 93 with something like this? If not, why not?
> > ```
> > const mask defined_bits = (F::cntrl | F::print | F::space | F::blank | F::lower | F::upper | F::alpha | F::digit | F::xdigit | F::punct);
> > assert((p[i] & defined_bits) == 0);  // no bits are set outside the mask
> > ```
> The new tests are a more lenient way of checking these features; this enforces more aspects of it - on platforms where we want to assume that much.
> 
> No, line 92 is not redundant with the other conditions. Lines 64-73 check that for each constant (which may be one or more bits), we either have some bits set (if expected) or none of the bits set. Line 92 checks that every single bit in the expected bitmasks are set.
> 
> So for a platform with `F::alpha = F::lower | F::upper`, you'd get a failure because while we expect `F::alpha` to be set, we don't expect _all_ bits of `F::alpha` to be set.
> 
> Regarding line 93, no, this doesn't check what you suggest. Previously, this checked that we _don't_ have bits set that we don't expect. But actually, our lines 64-73 checks that aspect already, so we can get rid of this one.
But on second thought, while it is a stricter check (so we don't lose any test strictness on existing supported platforms), I'm not sure that it adds much value to check that all bits of all the features are set either. So I would be totally ok with removing this ifdef altogether too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120802