This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [test] Fix the monetary locale curr_symbol test on Windows, Apple and FreeBSD
ClosedPublic

Authored by mstorsjo on Feb 25 2022, 1:31 AM.

Details

Summary

International currency symbols (like USD, EUR) are returned with a
trailing space, like "USD ", on previously supported Unix platforms.
On Windows, the locales return them without a trailing space.

Also adjust the test for expecting a different unicode sequence for
the national currency symbol for ru_RU.UTF-8 and zh_CN.UTF-8.

Diff Detail

Event Timeline

mstorsjo requested review of this revision.Feb 25 2022, 1:31 AM
mstorsjo created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2022, 1:31 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Seems reasonable to me. Some pre-existing nitpicks.

libcxx/test/std/localization/locale.categories/category.monetary/locale.moneypunct.byname/curr_symbol.pass.cpp
8–9

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?

168–170

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. :)

184–188

Could these be "\u00A5" and "\uFFE5" for clarity?

197–201

I think these should use \u not \x, too.

mstorsjo added inline comments.Feb 25 2022, 10:30 AM
libcxx/test/std/localization/locale.categories/category.monetary/locale.moneypunct.byname/curr_symbol.pass.cpp
8–9

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.

168–170

Oh, yes, the inconsistency annoyed me too :-)

184–188

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.

mstorsjo updated this revision to Diff 411525.Feb 25 2022, 2:21 PM

Adjusted the test so that it passes on Windows, current macOS and FreeBSD (to avoid leaving any unused #else clause that isn't used by any target at all).

Also made the escape sequences consistent; use \u0000 form for all wchar strings, and \x00 form for all UTF-8 strings (with comments explaining what unicode chars it corresponds to).

mstorsjo retitled this revision from [libcxx] [test] Fix the monetary locale curr_symbol test on Windows to [libcxx] [test] Fix the monetary locale curr_symbol test on Windows, Apple and FreeBSD.Feb 25 2022, 2:27 PM
mstorsjo updated this revision to Diff 411623.Feb 26 2022, 9:37 AM

Rerun CI after unrelated failures on main are fixed.

Anyone up for giving the full stamp of approval here? It has got a personal approval so far only.

Mordante accepted this revision.Feb 28 2022, 9:42 AM

LGTM!

This revision is now accepted and ready to land.Feb 28 2022, 9:42 AM