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


================
Comment at: libcxx/test/std/localization/locale.categories/category.ctype/facet.ctype.special/facet.ctype.char.statics/classic_table.pass.cpp:41-42
+#if defined(_MSVC_STL_VERSION)
+        // MS STL includes the _SPACE bit in F::blank
+        bool expect_blank = (i == ' ') || expect_space;
+#elif defined(_WIN32)
----------------
I'm OK with this, but personally I would have spelled out
```
        // MS STL includes the _SPACE bit in F::blank
        bool expect_blank = (9 <= i && i <= 13) || (i == ' ');
```
because it's cheap.


================
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);  // '{' .. '~'    }
+
----------------
Stray `}` at the end of the comment?
Over-helpful IDE closing your `'{` for you? ;)


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120802