[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:39
+
+        bool expect_space = ((9 <= i && i <= 13) || i == ' ');
+#if defined(_MSVC_STL_VERSION)
----------------
Quuxplusone wrote:
> (nit)
Right, thanks


================
Comment at: libcxx/test/std/localization/locale.categories/category.ctype/facet.ctype.special/facet.ctype.char.statics/classic_table.pass.cpp:62
+                         || (91 <= i && i <= 96)     // '[' .. '`'
+                         || (123 <= i && i <= 126);  // '{' .. '~'    }
+
----------------
Quuxplusone wrote:
> Stray `}` at the end of the comment?
> Over-helpful IDE closing your `'{` for you? ;)
Actually, there might have been a helpful IDE for whoever wrote this originally, I'm just bringing the typo along from the original version :P Will fix.


================
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;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120802