[libcxx-commits] [PATCH] D120547: [libcxx] [test] Fix the monetary locale curr_symbol test on Windows

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: libcxx/test/std/localization/locale.categories/category.monetary/locale.moneypunct.byname/curr_symbol.pass.cpp:9
 //
 // XFAIL: darwin
 //
----------------
Quuxplusone wrote:
> Any chance this passes on Apple now? If not, do you mind adding a one-line comment here since presumably you've already dug into it a little bit?
Not quite yet (as CI would error out then), but I can have a look if it’s easily fixable at the same time - and if not I can add a comment.


================
Comment at: libcxx/test/std/localization/locale.categories/category.monetary/locale.moneypunct.byname/curr_symbol.pass.cpp:162
         else
           assert(f.curr_symbol() == L" \x440\x443\x431");
+#elif defined(_WIN32)
----------------
Quuxplusone wrote:
> Pre-existing: it sure would be nice if this could be changed to `L" \u0440\u0443\u0431"` for clarity. I'm like 50% sure those mean the same thing. :)
Oh, yes, the inconsistency annoyed me too :-)


================
Comment at: libcxx/test/std/localization/locale.categories/category.monetary/locale.moneypunct.byname/curr_symbol.pass.cpp:178-182
+#ifdef _WIN32
+        assert(f.curr_symbol() == "\xC2\xA5");
+#else
         assert(f.curr_symbol() == "\xEF\xBF\xA5");
+#endif
----------------
Quuxplusone wrote:
> Could these be `"\u00A5"` and `"\uFFE5"` for clarity?
I think they could, but for UTF-8 strings, I’d actually prefer to stick consistently to a series of bytes, instead of `\u` escapes, as that makes the exact content a bit clearer (especially for cases when debugging the tests). But I guess I could add a comment with what Unicode sequence it corresponds to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120547