This is an archive of the discontinued LLVM Phabricator instance.

[libc++][AIX][test] Enable put_double/long_double locale tests
ClosedPublic

Authored by jsji on Feb 23 2022, 7:01 PM.

Details

Summary

AIX print -0.0 , inf, nan differently, which are causing the test
failures. We are OK for most other tests.

This patch remove the tests related these limitations conditionally on AIX,
so that we can enable the other tests to avoid losing test coverage.

The general direction is:

if strings don't differ between environments, keep the string literal "INF" and the padding, instead of folding them into variables.

Diff Detail

Event Timeline

jsji requested review of this revision.Feb 23 2022, 7:01 PM
jsji created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 23 2022, 7:01 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added inline comments.Mar 1 2022, 12:35 PM
libcxx/test/std/localization/locale.categories/category.numeric/locale.nm.put/facet.num.put.members/put_long_double.pass.cpp
22641

Can we instead fix those tests on AIX, i.e.

#ifdef _AIX
  assert(something)
#else
  assert(something else)
#endif
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 12:35 PM
jsji added inline comments.Mar 1 2022, 1:21 PM
libcxx/test/std/localization/locale.categories/category.numeric/locale.nm.put/facet.num.put.members/put_long_double.pass.cpp
22641

Sure.

jsji updated this revision to Diff 412429.Mar 2 2022, 8:19 AM

Address comments.

put_double.pass.cpp was already fixed (test removed) in https://reviews.llvm.org/D120022.

mstorsjo added inline comments.Mar 2 2022, 2:36 PM
libcxx/test/std/localization/locale.categories/category.numeric/locale.nm.put/facet.num.put.members/put_long_double.pass.cpp
8939

I think the general direction in earlier reviews (I think it might have been in D120022), is that for these strings that don't differ between environments, keep the string literal "INF" and the padding, instead of folding them into variables.

So here, the only thing that really differs is inf vs INF, then we would only need that single variable, but keeping the literal strings for everything else.

9174

I guess this is quite nitpicky, but many of these edits seem to be adding more inconsistent whitespace in the test - here there's suddenly two spaces between == and ninf_sign, and there's a trailing space before the ending parenthesis. I'd prefer if we'd avoid introducing extra inconsistencies.

10733

Why the extra std::string() here? A plain string literal like "NAN" should work just fine?

jsji updated this revision to Diff 412767.Mar 3 2022, 10:44 AM

Address comments.

Thanks for working on this, I like the direction it's going.
Please update the summary with the current direction.

libcxx/test/std/localization/locale.categories/category.numeric/locale.nm.put/facet.num.put.members/put_long_double.pass.cpp
8957

Please make sure you rebase your next iteration of this patch. I just committed a change to this line. I like to see the CI pass with those conflicts resolved.

10770

Please check the rest of the patch for similar unneeded spaces.

jsji edited the summary of this revision. (Show Details)Mar 3 2022, 3:12 PM
jsji updated this revision to Diff 413024.Mar 4 2022, 8:34 AM

Rebased and address comments.

Mordante accepted this revision as: Mordante.Mar 7 2022, 10:05 AM

LGTM, please wait for libc++ approval before landing this patch.

jsji added a comment.Mar 7 2022, 10:28 AM

LGTM, please wait for libc++ approval before landing this patch.

Thanks!

ldionne accepted this revision.Mar 7 2022, 11:00 AM
This revision is now accepted and ready to land.Mar 7 2022, 11:00 AM