This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [test] Fix get_string_en_US, get_long_double_en_US for Windows
ClosedPublic

Authored by mstorsjo on Mar 2 2022, 1:25 AM.

Details

Summary

In the en_US locale on Windows, negative currency amounts is formatted
as "($0.01)" instead of "-$0.01".

This is the same issue as D120798, but with a different fix strategy.

To avoid excessive amounts of ifdefs to get the right string form for
each of them, just ifdef out groups of tests that test negative values.
(This keeps the number of ifdefs needed more reasonable, as there's 2-3
strings per ifdef in this case.)

Alternatively, we could just keep the XFAIL without the FIXME part and
conclude that this is known to be different and we don't want the churn
to fix it.

Diff Detail

Event Timeline

mstorsjo created this revision.Mar 2 2022, 1:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 1:25 AM
mstorsjo requested review of this revision.Mar 2 2022, 1:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 1:25 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
mstorsjo updated this revision to Diff 412743.Mar 3 2022, 9:15 AM

Use a small local helper function for producing the expected negative value form.

Quuxplusone added inline comments.
libcxx/test/std/localization/locale.categories/category.monetary/locale.money.get/locale.money.get.members/get_long_double_en_US.pass.cpp
65

Could this go in LocaleHelpers instead? Or would that be problematic because the behavior depends on the specific locale too? (Or is that a good thing because it indicates that this function should actually be named currency_negate_en_US or something?)

Worth thinking about, but IMHO not worth my blocking this PR over. :)

mstorsjo added inline comments.Mar 3 2022, 10:29 AM
libcxx/test/std/localization/locale.categories/category.monetary/locale.money.get/locale.money.get.members/get_long_double_en_US.pass.cpp
65

Yeah it could go there with such a name too - making all the calls to it slightly longer and more verbose though. But possibly still tolerable.

Mordante accepted this revision.Mar 3 2022, 1:04 PM

Thanks for this improvement, LGTM!
Before committing, please update the summary and make sure the CI passes.

libcxx/test/std/localization/locale.categories/category.monetary/locale.money.get/locale.money.get.members/get_long_double_en_US.pass.cpp
65

Since the code is duplicated I also have a slight preference to move it, but non-blocking for me too.

This revision is now accepted and ready to land.Mar 3 2022, 1:04 PM
mstorsjo updated this revision to Diff 412838.Mar 3 2022, 2:13 PM

Move the negate helper function to locale_helpers.h, fix building without wchar. Will push tomorrow if CI is green.

This revision was landed with ongoing or failed builds.Mar 4 2022, 12:19 AM
This revision was automatically updated to reflect the committed changes.