Page MenuHomePhabricator

[SystemZ][z/OS][libcxx]: fix libcxx test cases failed on ebcdic mode on z/OS

Authored by NancyWang2222 on Jun 22 2021, 2:13 PM.



This PR is to fix 2 libcxx test cases, test cases assumed 'a' > 'A' which is not case in z/OS platform on ebcdic mode, modified test cases to compare between upper letters or lower letters, or digits so ordering will be true for all platform.

Diff Detail

Event Timeline

NancyWang2222 requested review of this revision.Jun 22 2021, 2:13 PM
NancyWang2222 created this revision.

ping ;) can I get review for this ?

muiez accepted this revision.Jun 23 2021, 12:34 PM


This revision is now accepted and ready to land.Jun 23 2021, 12:34 PM
muiez added a subscriber: ldionne.Jun 23 2021, 1:03 PM

Maybe we should also get a group review from @libc++? @ldionne

NancyWang2222 added a subscriber: Restricted Project.Jun 23 2021, 1:18 PM
muiez added a reviewer: Restricted Project.Jun 23 2021, 1:19 PM
Quuxplusone requested changes to this revision.Jun 23 2021, 3:48 PM
Quuxplusone added a subscriber: Quuxplusone.
Quuxplusone added inline comments.

I think I'd prefer to see

char lo = std::min('a', 'A');
char hi = std::max('a', 'A');
assert(std::char_traits<char>::lt(lo, hi));

With just your new tests, a case-insensitive comparison would pass the tests, and I think the intent of these tests is specifically to make sure a case-insensitive comparison won't pass.


Please use the same set of test inputs here as in the char test above. (Yeah, the old code also failed at this; but we can be better. :))


Btw, offhand I suspect a lot of (libc++ and otherwise) code won't work on non-ASCII platforms; would it perhaps make more engineering sense to look into build-system ways to make these tests run in ASCII mode? For example, maybe the test runner should be setting LC_ALL in the environment, or something like that.
Am I correct that you didn't need to make any similar changes to char.traits.specializations.char{8,16,32}_t/lt.pass.cpp because those three character types already Do The Right Thing even in your platform's EBCDIC mode?

This revision now requires changes to proceed.Jun 23 2021, 3:48 PM
SeanP added a subscriber: SeanP.Jun 23 2021, 6:35 PM
SeanP added inline comments.

I think the assertion Nancy has is essentially the same as the three liner you are proposing.

To catch the case-insensitive comparison we should add:

assert(std::char_traits<char>::lt('A','a') == !std::char_traits<char>::lt('a','A'));


It is important to run tests for the non-ASCII flavour of libc++ as well as the ASCII flavour. The good news is check-cxx works for both flavours. It is only a few tests like this one that has a built in assumption about the ordering of the subgroups (eg. lower case letters) within the character encoding that are failing.

You are correct, the char{8,16,32}_t tests would pass because they use Unicode literals. The encoding for these are defined to have the ASCII encoding for the single character literals.

NancyWang2222 added inline comments.Jun 24 2021, 7:12 AM

As Sean suggested. I can add assert(std::char_traits<char>::lt('A','a') == !std::char_traits<char>::lt('a','A'));


I can use same tests for both except one is char another is wchar_t


I guess that's acceptable, although I would still prefer a version that explicitly tests that lt returns the intended answer.
Actually, how about just doing this?

assert(std::char_traits<char>::lt('A', 'a') == ('A' < 'a'));
assert(std::char_traits<char>::lt('a', 'A') == ('a' < 'A'));
assert(std::char_traits<char>::lt(' ', 'A') == (' ' < 'A'));
assert(std::char_traits<char>::lt('A', '~') == ('A' < '~'));

Then you can keep all the test cases from the left-hand side — you just have to add the appropriate == ('x' < 'y') for each of them. (Your new strictly-alphanumeric test cases are good additions; but I did think it was a pity to lose the ones testing punctuation, and this way we get to keep them.)


Yes, of course.

  1. added assert(std::char_traits<char>::lt('A','a') == !std::char_traits<char>::lt('a','A')); for case insensitive test.
  2. make content of 2 test cases same except one testing char type another one testing wchar_t
NancyWang2222 marked 7 inline comments as done.Jun 24 2021, 9:18 AM
NancyWang2222 added inline comments.Jun 24 2021, 11:11 AM

sure I can use this pattern. Thanks

use new assert pattern so that we can keep old tests

hi Arthur O'Dwyer , can you kindly review it again? Thanks

Quuxplusone accepted this revision.Jun 28 2021, 7:02 AM

Especially if the !s are removed, LGTM at this point.
I'm taking the liberty of marking it accepted for libc++ too, since I think it's fine to land. (If @ldionne overrules this, listen to him.)


At this point I recommend removing the ! from both sides. Ditto throughout.

This revision is now accepted and ready to land.Jun 28 2021, 7:02 AM
NancyWang2222 added inline comments.Jun 28 2021, 7:21 AM

sure I will remove "!"

Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2021, 11:05 AM

I'm fine with this change because of this:

@SeanP said:
It is only a few tests like this one that has a built in assumption about the ordering of the subgroups (eg. lower case letters) within the character encoding that are failing.

Otherwise, I think it would have been useful to have a discussion about how to handle those gracefully without introducing a lot of complexity into the test suite. If that's just a few changes, I think it's not worth wasting time over that and we should just find a simple way to fix the tests on a case by case basis.