[libcxx-commits] [PATCH] D120796: [libcxx] Fix the ctype `is` (pointer version) function for Windows

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/localization/locale.categories/category.ctype/locale.ctype.byname/is_1.pass.cpp:110-112
+            // On Windows, in the "C" locale, \x00DA is interpreted according
+            // to the current code page, where this char can be considered an
+            // uppercase alphabetical character.
----------------
mstorsjo wrote:
> Quuxplusone wrote:
> > This comment is too vague for my liking. I infer that "can be" means sometimes it is and sometimes it isn't. I'd rather see something very specific, like
> > ```
> > // On Windows, this depends on the current codepage.
> > // If the codepage is CP-437, \x00DA is U+250C BOX DRAWINGS LIGHT DOWN AND RIGHT.
> > // If the codepage is CP-1252, \x00DA is U+00DA CAPITAL LETTER U WITH ACUTE.
> > ```
> > And then I'm not actually sure why the non-Windows behavior claims that U+00DA is //not// alpha+upper.
> I think in the non-Windows case, the "C" locale might correspond to plain ASCII or something like that, where `\x00DA` isn't considered an alphabetical character.
Nit: for readability, I'd prefer linebreaks before each "If", and ideally no other linebreaks.

I'm still confused as to why the comment says "it depends" but the code on lines 114-115 is very clearly assuming that the character //is// U+00DA CAPITAL LETTER U WITH ACUTE. Is this because the comment is wrong? or because the test runner forces a specific codepage? or because we just luck into a specific codepage and this is super fragile? (Ditto throughout.)


================
Comment at: libcxx/test/std/localization/locale.categories/category.ctype/locale.ctype.byname/scan_not.pass.cpp:75
             assert(f.scan_not(F::graph, in.data(), in.data() + in.size()) - in.data() == 0);
+#endif
         }
----------------
Pre-existing, no action required: This test seems silly. All of the expected results are `0`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120796