Skip to content

Conversation

zibi2
Copy link
Contributor

@zibi2 zibi2 commented May 31, 2024

This PR carves out small portion of the test in subject to avoid the following failure when unicode is not available.

# | Assertion failure: result == expected .../formatter.char.funsigned-char.pass.cpp 56
# |
# | Format string   ?}
# | Expected output '\x{80}'
# | Actual output   '�'

This was traced down to different definition of __code_point_view::__consume() under macro_LIBCXX_HAS_NO_UNICODE which is called inside __formatter::__escape(). The __consume() returns __ok and code assumes that escaped sequence was already written but it is not., thus the failure. Here is the snippen code we fall into:

    typename __unicode::__consume_result __result = __view.__consume();
    if (__result.__status == __unicode::__consume_result::__ok) {
      __escape = __formatter::__is_escaped_sequence_written(__str, __result.__code_point, __escape, __mark);

@zibi2 zibi2 requested a review from mordante May 31, 2024 20:18
@zibi2 zibi2 self-assigned this May 31, 2024
@zibi2 zibi2 requested a review from a team as a code owner May 31, 2024 20:18
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label May 31, 2024
@llvmbot
Copy link
Member

llvmbot commented May 31, 2024

@llvm/pr-subscribers-libcxx

Author: Zibi Sarbinowski (zibi2)

Changes

This PR carves out small portion of the test in subject to avoid the following failure when unicode is not available.

# | Assertion failure: result == expected .../formatter.char.funsigned-char.pass.cpp 56
# |
# | Format string   ?}
# | Expected output '\x{80}'
# | Actual output   '�'

This was traced down to different definition of __code_point_view::__consume() under macro_LIBCXX_HAS_NO_UNICODE which is called inside __formatter::__escape(). The __consume() returns __ok and code assumes that escaped sequence was already written but it is not., thus the failure. Here is the snippen code we fall into:

    typename __unicode::__consume_result __result = __view.__consume();
    if (__result.__status == __unicode::__consume_result::__ok) {
      __escape = __formatter::__is_escaped_sequence_written(__str, __result.__code_point, __escape, __mark);

Full diff: https://github.com/llvm/llvm-project/pull/94044.diff

1 Files Affected:

  • (modified) libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.char.funsigned-char.pass.cpp (+4-2)
diff --git a/libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.char.funsigned-char.pass.cpp b/libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.char.funsigned-char.pass.cpp
index 9c31ecad85eac..643efe00e3fc2 100644
--- a/libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.char.funsigned-char.pass.cpp
+++ b/libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.char.funsigned-char.pass.cpp
@@ -71,16 +71,18 @@ void test() {
 #if TEST_STD_VER > 20
   test(STR(R"('\u{0}')"), STR("?}"), '\x00');
   test(STR("'a'"), STR("?}"), 'a');
+#  ifndef TEST_HAS_NO_UNICODE
   if constexpr (std::same_as<CharT, char>) {
     test(STR(R"('\x{80}')"), STR("?}"), '\x80');
     test(STR(R"('\x{ff}')"), STR("?}"), '\xff');
   }
-#  ifndef TEST_HAS_NO_WIDE_CHARACTERS
+#    ifndef TEST_HAS_NO_WIDE_CHARACTERS
   else {
     test(STR(R"('\u{80}')"), STR("?}"), '\x80');
     test(STR("'\u00ff'"), STR("?}"), '\xff');
   }
-#  endif // TEST_HAS_NO_WIDE_CHARACTERS
+#    endif // TEST_HAS_NO_WIDE_CHARACTERS
+#  endif // TEST_HAS_NO_UNICODE
 #endif   // TEST_STD_VER > 20
 
   test(STR("10000000"), STR("b}"), char(128));

@zibi2
Copy link
Contributor Author

zibi2 commented May 31, 2024

The other alternative would be to diable entire test for no unicode mode only which seems like a big hammer for me.

@zibi2 zibi2 changed the title [libc++][z/OS] Disable porsion of formatter.char.funsigned-char.pass.cpp for no unicode [libc++][z/OS] Disable portion of formatter.char.funsigned-char.pass.cpp for no unicode May 31, 2024

This comment was marked as resolved.

@zibi2 zibi2 requested review from redstar and fanbo-meng May 31, 2024 20:22
@zibi2
Copy link
Contributor Author

zibi2 commented Jun 6, 2024

ping @mordante

Copy link
Member

@mordante mordante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM!

@zibi2 zibi2 merged commit 47afa10 into llvm:main Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants