[libc++][test] Avoid preprocessor directives in macro argument lists #73440
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…n function-like macro argument list is undefined behavior
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Stephan T. Lavavej (StephanTLavavej) ChangesFound while running libc++'s test suite with MSVC's STL. MSVC has a level 1 "warning C5101: use of preprocessor directive in function-like macro argument list is undefined behavior". I don't know why Clang doesn't complain about this. There are some formatting tests which densely interleave preprocessor directives within function-like macros, and they would need invasive changes. For now, I'm just skipping those tests. However, a few tests were only slightly affected, and I was able to add a new test macro Full diff: https://github.com/llvm/llvm-project/pull/73440.diff 5 Files Affected:
diff --git a/libcxx/test/std/input.output/iostream.format/print.fun/print.file.pass.cpp b/libcxx/test/std/input.output/iostream.format/print.fun/print.file.pass.cpp
index 4a397d3e3d6328f..3edc0e2245d649e 100644
--- a/libcxx/test/std/input.output/iostream.format/print.fun/print.file.pass.cpp
+++ b/libcxx/test/std/input.output/iostream.format/print.fun/print.file.pass.cpp
@@ -94,11 +94,8 @@ static void test_read_only() {
TEST_VALIDATE_EXCEPTION(
std::system_error,
[&]([[maybe_unused]] const std::system_error& e) {
-#ifdef _AIX
- [[maybe_unused]] std::string_view what{"failed to write formatted output: Broken pipe"};
-#else
- [[maybe_unused]] std::string_view what{"failed to write formatted output: Operation not permitted"};
-#endif
+ [[maybe_unused]] std::string_view what{
+ "failed to write formatted output: " TEST_IF_AIX("Broken pipe", "Operation not permitted")};
TEST_LIBCPP_REQUIRE(
e.what() == what,
TEST_WRITE_CONCATENATED("\nExpected exception ", what, "\nActual exception ", e.what(), '\n'));
diff --git a/libcxx/test/std/input.output/iostream.format/print.fun/println.file.pass.cpp b/libcxx/test/std/input.output/iostream.format/print.fun/println.file.pass.cpp
index ebdddd074faf51c..07272ebb57e5fc1 100644
--- a/libcxx/test/std/input.output/iostream.format/print.fun/println.file.pass.cpp
+++ b/libcxx/test/std/input.output/iostream.format/print.fun/println.file.pass.cpp
@@ -97,11 +97,8 @@ static void test_read_only() {
TEST_VALIDATE_EXCEPTION(
std::system_error,
[&]([[maybe_unused]] const std::system_error& e) {
-#ifdef _AIX
- [[maybe_unused]] std::string_view what{"failed to write formatted output: Broken pipe"};
-#else
- [[maybe_unused]] std::string_view what{"failed to write formatted output: Operation not permitted"};
-#endif
+ [[maybe_unused]] std::string_view what{
+ "failed to write formatted output: " TEST_IF_AIX("Broken pipe", "Operation not permitted")};
TEST_LIBCPP_REQUIRE(
e.what() == what,
TEST_WRITE_CONCATENATED("\nExpected exception ", what, "\nActual exception ", e.what(), '\n'));
diff --git a/libcxx/test/std/input.output/iostream.format/print.fun/vprint_nonunicode.file.pass.cpp b/libcxx/test/std/input.output/iostream.format/print.fun/vprint_nonunicode.file.pass.cpp
index 9eb85f3b7b2d874..edc8bb3f543c466 100644
--- a/libcxx/test/std/input.output/iostream.format/print.fun/vprint_nonunicode.file.pass.cpp
+++ b/libcxx/test/std/input.output/iostream.format/print.fun/vprint_nonunicode.file.pass.cpp
@@ -103,11 +103,8 @@ static void test_read_only() {
TEST_VALIDATE_EXCEPTION(
std::system_error,
[&]([[maybe_unused]] const std::system_error& e) {
-#ifdef _AIX
- [[maybe_unused]] std::string_view what{"failed to write formatted output: Broken pipe"};
-#else
- [[maybe_unused]] std::string_view what{"failed to write formatted output: Operation not permitted"};
-#endif
+ [[maybe_unused]] std::string_view what{
+ "failed to write formatted output: " TEST_IF_AIX("Broken pipe", "Operation not permitted")};
TEST_LIBCPP_REQUIRE(
e.what() == what,
TEST_WRITE_CONCATENATED("\nExpected exception ", what, "\nActual exception ", e.what(), '\n'));
diff --git a/libcxx/test/std/input.output/iostream.format/print.fun/vprint_unicode.file.pass.cpp b/libcxx/test/std/input.output/iostream.format/print.fun/vprint_unicode.file.pass.cpp
index 28379b9db50ed58..bd9b99166922ff0 100644
--- a/libcxx/test/std/input.output/iostream.format/print.fun/vprint_unicode.file.pass.cpp
+++ b/libcxx/test/std/input.output/iostream.format/print.fun/vprint_unicode.file.pass.cpp
@@ -110,11 +110,8 @@ static void test_read_only() {
TEST_VALIDATE_EXCEPTION(
std::system_error,
[&]([[maybe_unused]] const std::system_error& e) {
-#ifdef _AIX
- [[maybe_unused]] std::string_view what{"failed to write formatted output: Broken pipe"};
-#else
- [[maybe_unused]] std::string_view what{"failed to write formatted output: Operation not permitted"};
-#endif
+ [[maybe_unused]] std::string_view what{
+ "failed to write formatted output: " TEST_IF_AIX("Broken pipe", "Operation not permitted")};
TEST_LIBCPP_REQUIRE(
e.what() == what,
TEST_WRITE_CONCATENATED("\nExpected exception ", what, "\nActual exception ", e.what(), '\n'));
diff --git a/libcxx/test/support/test_macros.h b/libcxx/test/support/test_macros.h
index e549ec2ca7a1a71..f3c6d8080ff6d83 100644
--- a/libcxx/test/support/test_macros.h
+++ b/libcxx/test/support/test_macros.h
@@ -443,4 +443,10 @@ inline void DoNotOptimize(Tp const& value) {
# define TEST_WORKAROUND_BUG_109234844_WEAK /* nothing */
#endif
+#ifdef _AIX
+# define TEST_IF_AIX(arg_true, arg_false) arg_true
+#else
+# define TEST_IF_AIX(arg_true, arg_false) arg_false
+#endif
+
#endif // SUPPORT_TEST_MACROS_HPP
|
mordante
approved these changes
Nov 27, 2023
|
Thanks! I filed a Clang bug report, after looking up the relevant Standardese saying it's UB. The CI failures here are spurious, so I'll go ahead and merge. BTW, the remaining affected tests (that had way more occurrences that I couldn't easily fix) are: std/time/time.syn/formatter.duration.pass.cpp |
|
Thanks for filing a Clang bug, I files a libc++ bug for myself to track the libc++ issues. |
ldionne
pushed a commit
that referenced
this pull request
May 28, 2024
Found while running libc++'s tests with MSVC's STL. * Avoid MSVC warning C5101: use of preprocessor directive in function-like macro argument list is undefined behavior. + We can easily make this portable by extracting `const bool is_newlib`. + Followup to #73440. + See #73598. + See #73836. * Avoid MSVC warning C4267: 'return': conversion from 'size_t' to 'int', possible loss of data. + This warning is valid, but harmless for the test, so `static_cast<int>` will avoid it. * Avoid MSVC warning C4146: unary minus operator applied to unsigned type, result still unsigned. + This warning is also valid (the scenario is sometimes intentional, but surprising enough that it's worth warning about). This is a C++17 test, so we can easily avoid it by testing `is_signed_v` at compile-time before testing `m < 0` and `n < 0` at run-time. * Silence MSVC warning C4310: cast truncates constant value. + These warnings are being emitted by `T(255)`. Disabling the warning is simpler than attempting to restructure the code. + Followup to #79791. * MSVC no longer emits warning C4521: multiple copy constructors specified. + This warning was removed from the compiler, since at least 2021-12-09.
vg0204
pushed a commit
to vg0204/llvm-project
that referenced
this pull request
May 29, 2024
Found while running libc++'s tests with MSVC's STL. * Avoid MSVC warning C5101: use of preprocessor directive in function-like macro argument list is undefined behavior. + We can easily make this portable by extracting `const bool is_newlib`. + Followup to llvm#73440. + See llvm#73598. + See llvm#73836. * Avoid MSVC warning C4267: 'return': conversion from 'size_t' to 'int', possible loss of data. + This warning is valid, but harmless for the test, so `static_cast<int>` will avoid it. * Avoid MSVC warning C4146: unary minus operator applied to unsigned type, result still unsigned. + This warning is also valid (the scenario is sometimes intentional, but surprising enough that it's worth warning about). This is a C++17 test, so we can easily avoid it by testing `is_signed_v` at compile-time before testing `m < 0` and `n < 0` at run-time. * Silence MSVC warning C4310: cast truncates constant value. + These warnings are being emitted by `T(255)`. Disabling the warning is simpler than attempting to restructure the code. + Followup to llvm#79791. * MSVC no longer emits warning C4521: multiple copy constructors specified. + This warning was removed from the compiler, since at least 2021-12-09.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Found while running libc++'s test suite with MSVC's STL.
MSVC has a level 1 "warning C5101: use of preprocessor directive in function-like macro argument list is undefined behavior". I don't know why Clang doesn't complain about this.
There are some formatting tests which densely interleave preprocessor directives within function-like macros, and they would need invasive changes. For now, I'm just skipping those tests.
However, a few tests were only slightly affected, and I was able to add a new test macro
TEST_IF_AIXto make them portable.