This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [test] Fix the monetary locale negative_sign test for en_US.UTF-8 on Windows
ClosedPublic

Authored by mstorsjo on Feb 25 2022, 2:09 AM.

Details

Summary

On Windows, the en_US.UTF-8 locale returns n_sign_posn == 0, which
means that the sign for a negative currency is parentheses around
the whole value, instead of a leading minus.

Diff Detail

Event Timeline

mstorsjo requested review of this revision.Feb 25 2022, 2:09 AM
mstorsjo created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2022, 2:09 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
mstorsjo updated this revision to Diff 411385.Feb 25 2022, 5:22 AM

Simplify the patch significantly, fixing building in configurations without wchar available.

Quuxplusone accepted this revision.Feb 25 2022, 7:42 AM

Seems fine to me. My only suggestion is that I imagine there must be other tests that need updating in the same way for actually formatting negative numbers, like

        Fwt f(LOCALE_en_US_UTF_8, 1);
#if defined(_WIN32)
        assert(f.whateverformatmoney(-42) == L"(42)");
#else
        assert(f.whateverformatmoney(-42) == L"-42");
#endif

and maybe you could hit those tests in this same PR?

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

Seems fine to me. My only suggestion is that I imagine there must be other tests that need updating in the same way for actually formatting negative numbers, like

        Fwt f(LOCALE_en_US_UTF_8, 1);
#if defined(_WIN32)
        assert(f.whateverformatmoney(-42) == L"(42)");
#else
        assert(f.whateverformatmoney(-42) == L"-42");
#endif

and maybe you could hit those tests in this same PR?

Yes, there’s actually a bunch of them. Unfortunately, some of them require a whole lot of churn in the tests, so I’m a bit undecided about some of them, whether it’s worth to fix them, or just change the XFAIL to a non-FIXME version, with an explanation. But this one seemed tolerable enough at least.