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

[NancyWang2222]

Summary

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.

[NancyWang2222]

ping ;) can I get review for this ?

[muiez]

LGTM

[muiez]

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

[• Quuxplusone]

libcxx/test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.char/lt.pass.cpp

25–27

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.

libcxx/test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.wchar.t/lt.pass.cpp

28

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

35

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?

[SeanP]

libcxx/test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.char/lt.pass.cpp

25–27

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'));

libcxx/test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.wchar.t/lt.pass.cpp

35

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]

libcxx/test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.char/lt.pass.cpp

25–27

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

libcxx/test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.wchar.t/lt.pass.cpp

28

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

[• Quuxplusone]

libcxx/test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.char/lt.pass.cpp

25–27

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

libcxx/test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.wchar.t/lt.pass.cpp

28

Yes, of course.

[NancyWang2222]

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]

libcxx/test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.char/lt.pass.cpp

25–27

sure I can use this pattern. Thanks

[NancyWang2222]

use new assert pattern so that we can keep old tests

[NancyWang2222]

ping :)

[NancyWang2222]

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

[• Quuxplusone]

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

libcxx/test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.char/lt.pass.cpp

23

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

[NancyWang2222]

libcxx/test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.char/lt.pass.cpp

23

sure I will remove "!"

[ldionne]

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.

libcxx/test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.char/lt.pass.cpp

 
 #include <string>
 #include <cassert>
 
 #include "test_macros.h"
 
 int main(int, char**)
 {
-    assert( std::char_traits<char>::lt('\0', 'A'));
-    assert(!std::char_traits<char>::lt('A', '\0'));
+  assert(std::char_traits<char>::lt('\0', 'A') == ('\0' < 'A'));
+  assert(std::char_traits<char>::lt('A', '\0') == ('A' < '\0'));
 
-    assert(!std::char_traits<char>::lt('a', 'a'));
-    assert( std::char_traits<char>::lt('A', 'a'));
-    assert(!std::char_traits<char>::lt('a', 'A'));
+  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') == ('a' < 'A'));
 
-    assert( std::char_traits<char>::lt('a', 'z'));
-    assert( std::char_traits<char>::lt('A', 'Z'));
+  assert(std::char_traits<char>::lt('a', 'z') == ('a' < 'z'));
+  assert(std::char_traits<char>::lt('A', 'Z') == ('A' < 'Z'));
 
-    assert( std::char_traits<char>::lt(' ', 'A'));
-    assert( std::char_traits<char>::lt('A', '~'));
+  assert(std::char_traits<char>::lt(' ', 'A') == (' ' < 'A'));
+  assert(std::char_traits<char>::lt('A', '~') == ('A' < '~'));
 
   return 0;
 }

libcxx/test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.wchar.t/lt.pass.cpp

 
 #include <string>
 #include <cassert>
 
 #include "test_macros.h"
 
 int main(int, char**)
 {
-    assert(!std::char_traits<wchar_t>::lt(L'a', L'a'));
-    assert( std::char_traits<wchar_t>::lt(L'A', L'a'));
+  assert(std::char_traits<wchar_t>::lt(L'\0', L'A') == (L'\0' < L'A'));
+  assert(std::char_traits<wchar_t>::lt(L'A', L'\0') == (L'A' < L'\0'));
+
+  assert(std::char_traits<wchar_t>::lt(L'a', L'a') == (L'a' < L'a'));
+  assert(std::char_traits<wchar_t>::lt(L'A', L'a') == (L'A' < L'a'));
+  assert(std::char_traits<wchar_t>::lt(L'a', L'A') == (L'a' < L'A'));
+
+  assert(std::char_traits<wchar_t>::lt(L'a', L'z') == (L'a' < L'z'));
+  assert(std::char_traits<wchar_t>::lt(L'A', L'Z') == (L'A' < L'Z'));
+
+  assert(std::char_traits<wchar_t>::lt(L' ', L'A') == (L' ' < L'A'));
+  assert(std::char_traits<wchar_t>::lt(L'A', L'~') == (L'A' < L'~'));
 
   return 0;
 }