Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc++][test] ADDITIONAL_COMPILE_FLAGS should be a space-separated list #73541

Merged
merged 6 commits into from
Nov 30, 2023

Conversation

StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Nov 27, 2023

Found while running libc++'s test suite with MSVC's STL.

ADDITIONAL_COMPILE_FLAGS is a ParserKind.LIST:

lit.TestRunner.IntegratedTestKeywordParser(
"ADDITIONAL_COMPILE_FLAGS:",
lit.TestRunner.ParserKind.LIST,
initial_value=additionalCompileFlags,
),

With a comma-separated example:

// ADDITIONAL_COMPILE_FLAGS: flag1, flag2, flag3
This directive will cause the provided flags to be added to the
%{compile_flags} substitution for the test that contains it. This
allows adding special compilation flags without having to use a
.sh.cpp test, which would be more powerful but perhaps overkill.

And comma-separated test coverage:

// ADDITIONAL_COMPILE_FLAGS: -foo
// ADDITIONAL_COMPILE_FLAGS: -bar
// ADDITIONAL_COMPILE_FLAGS: -baz, -foom
// RUN: echo "%{compile_flags}" | grep -e '-foo -bar -baz -foom'

Because the machinery splits on commas:

elif kind == ParserKind.LIST:
self.parser = self._handleList

def _handleList(line_number, line, output):
"""A parser for LIST type keywords"""
if output is None:
output = []
output.extend([s.strip() for s in line.split(",")])
return output

However, most (although not all) usage of ADDITIONAL_COMPILE_FLAGS is treating it as space-separated. That apparently works in the normal Clang environment, but in my exotic configuration it causes "-DMEOW -DWOOF" to be passed as a single argument to MSVC, which then emits "warning C5102: ignoring invalid command-line macro definition '_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT'", causing test failures due to warnings-as-errors.

This PR changes ADDITIONAL_COMPILE_FLAGS to actually be parsed as a space-separated list, and changes the few uses/examples that had commas.

Note: This gets my tests passing, and I tried to find everything I needed to change in the Python machinery, but I could have missed something. In particular, I tried to add good test coverage to llvm/utils/lit/tests/unit/TestRunner.py (intentionally verifying that commas are treated as normal characters) and llvm/utils/lit/tests/Inputs/testrunner-custom-parsers/test.txt but I'm uncertain whether that's actually being picked up because I found no other mentions of the previously existing test_lists.

Fixes MSVC warning C5102: ignoring invalid command-line macro definition '_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT'

See libcxx/utils/libcxx/test/format.py where ADDITIONAL_COMPILE_FLAGS is a ParserKind.LIST with an example line:
        // ADDITIONAL_COMPILE_FLAGS: flag1, flag2, flag3

See libcxx/test/libcxx/selftest/additional_compile_flags/substitutes-in-run.sh.cpp for test coverage.

See llvm/utils/lit/lit/TestRunner.py where ParserKind.LIST splits on commas.
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner November 27, 2023 16:39
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Nov 27, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 27, 2023

@llvm/pr-subscribers-testing-tools

@llvm/pr-subscribers-libcxx

Author: Stephan T. Lavavej (StephanTLavavej)

Changes

Found while running libc++'s test suite with MSVC's STL.

ADDITIONAL_COMPILE_FLAGS is a ParserKind.LIST:

lit.TestRunner.IntegratedTestKeywordParser(
"ADDITIONAL_COMPILE_FLAGS:",
lit.TestRunner.ParserKind.LIST,
initial_value=additionalCompileFlags,
),

With a comma-separated example:

// ADDITIONAL_COMPILE_FLAGS: flag1, flag2, flag3
This directive will cause the provided flags to be added to the
%{compile_flags} substitution for the test that contains it. This
allows adding special compilation flags without having to use a
.sh.cpp test, which would be more powerful but perhaps overkill.

And comma-separated test coverage:

// ADDITIONAL_COMPILE_FLAGS: -foo
// ADDITIONAL_COMPILE_FLAGS: -bar
// ADDITIONAL_COMPILE_FLAGS: -baz, -foom
// RUN: echo "%{compile_flags}" | grep -e '-foo -bar -baz -foom'

Because the machinery splits on commas:

elif kind == ParserKind.LIST:
self.parser = self._handleList

def _handleList(line_number, line, output):
"""A parser for LIST type keywords"""
if output is None:
output = []
output.extend([s.strip() for s in line.split(",")])
return output

However, most (although not all) usage of ADDITIONAL_COMPILE_FLAGS is treating it as space-separated. That apparently works in the normal Clang environment, but in my exotic configuration it causes "-DMEOW -DWOOF" to be passed as a single argument to MSVC, which then emits "warning C5102: ignoring invalid command-line macro definition '_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT'", causing test failures due to warnings-as-errors.

This PR adds commas to all ADDITIONAL_COMPILE_FLAGS defining multiple macros in libcxx/test/std. I'm not changing a few lines that say -Xclang -verify-ignore-unexpected=error, nor am I changing anything in libcxx/test/libcxx which we don't use.


Patch is 33.81 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/73541.diff

45 Files Affected:

  • (modified) libcxx/test/std/input.output/file.streams/fstreams/ifstream.members/buffered_reads.pass.cpp (+1-1)
  • (modified) libcxx/test/std/input.output/file.streams/fstreams/ofstream.members/buffered_writes.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locale.categories/category.ctype/locale.codecvt/locale.codecvt.members/utf_sanity_check.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locale.stdcvt/codecvt_mode.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locale.stdcvt/codecvt_utf16.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_always_noconv.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_encoding.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_in.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_length.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_max_length.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_out.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_unshift.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locale.stdcvt/codecvt_utf8.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_always_noconv.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_encoding.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_in.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_length.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_max_length.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_out.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_unshift.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_always_noconv.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_encoding.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_in.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_length.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_max_length.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_out.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_unshift.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locales/locale.convenience/conversions/conversions.buffer/ctor.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locales/locale.convenience/conversions/conversions.buffer/overflow.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locales/locale.convenience/conversions/conversions.buffer/pbackfail.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locales/locale.convenience/conversions/conversions.buffer/rdbuf.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locales/locale.convenience/conversions/conversions.buffer/state.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locales/locale.convenience/conversions/conversions.buffer/test.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locales/locale.convenience/conversions/conversions.buffer/underflow.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locales/locale.convenience/conversions/conversions.string/converted.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locales/locale.convenience/conversions/conversions.string/ctor_codecvt.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locales/locale.convenience/conversions/conversions.string/ctor_codecvt_state.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locales/locale.convenience/conversions/conversions.string/ctor_copy.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locales/locale.convenience/conversions/conversions.string/ctor_err_string.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locales/locale.convenience/conversions/conversions.string/from_bytes.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locales/locale.convenience/conversions/conversions.string/state.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locales/locale.convenience/conversions/conversions.string/to_bytes.pass.cpp (+1-1)
  • (modified) libcxx/test/std/localization/locales/locale.convenience/conversions/conversions.string/types.pass.cpp (+1-1)
  • (modified) libcxx/test/std/strings/basic.string/string.capacity/reserve.pass.cpp (+1-1)
  • (modified) libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.inv/invoke.pass.cpp (+1-1)
diff --git a/libcxx/test/std/input.output/file.streams/fstreams/ifstream.members/buffered_reads.pass.cpp b/libcxx/test/std/input.output/file.streams/fstreams/ifstream.members/buffered_reads.pass.cpp
index d57b7c20a2da272..30be46cd94c7700 100644
--- a/libcxx/test/std/input.output/file.streams/fstreams/ifstream.members/buffered_reads.pass.cpp
+++ b/libcxx/test/std/input.output/file.streams/fstreams/ifstream.members/buffered_reads.pass.cpp
@@ -6,7 +6,7 @@
 //
 //===----------------------------------------------------------------------===//
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 // UNSUPPORTED: c++03
 
 // <fstream>
diff --git a/libcxx/test/std/input.output/file.streams/fstreams/ofstream.members/buffered_writes.pass.cpp b/libcxx/test/std/input.output/file.streams/fstreams/ofstream.members/buffered_writes.pass.cpp
index e7820739505106a..5ee297ee2571cb5 100644
--- a/libcxx/test/std/input.output/file.streams/fstreams/ofstream.members/buffered_writes.pass.cpp
+++ b/libcxx/test/std/input.output/file.streams/fstreams/ofstream.members/buffered_writes.pass.cpp
@@ -6,7 +6,7 @@
 //
 //===----------------------------------------------------------------------===//
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 // UNSUPPORTED: c++03
 
 // <fstream>
diff --git a/libcxx/test/std/localization/locale.categories/category.ctype/locale.codecvt/locale.codecvt.members/utf_sanity_check.pass.cpp b/libcxx/test/std/localization/locale.categories/category.ctype/locale.codecvt/locale.codecvt.members/utf_sanity_check.pass.cpp
index 62fce70052c2a02..30468abcdd6326c 100644
--- a/libcxx/test/std/localization/locale.categories/category.ctype/locale.codecvt/locale.codecvt.members/utf_sanity_check.pass.cpp
+++ b/libcxx/test/std/localization/locale.categories/category.ctype/locale.codecvt/locale.codecvt.members/utf_sanity_check.pass.cpp
@@ -9,7 +9,7 @@
 // XFAIL: availability-char8_t_support-missing
 
 // This test runs in C++20, but we have deprecated codecvt<char(16|32), char, mbstate_t> in C++20.
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 
 // <locale>
 
diff --git a/libcxx/test/std/localization/locale.stdcvt/codecvt_mode.pass.cpp b/libcxx/test/std/localization/locale.stdcvt/codecvt_mode.pass.cpp
index a8a05da969da897..ec05818a946f85e 100644
--- a/libcxx/test/std/localization/locale.stdcvt/codecvt_mode.pass.cpp
+++ b/libcxx/test/std/localization/locale.stdcvt/codecvt_mode.pass.cpp
@@ -8,7 +8,7 @@
 
 // <codecvt>
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 
 // enum codecvt_mode
 // {
diff --git a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16.pass.cpp b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16.pass.cpp
index 229c634116796ff..4fbcb10bd1fbf59 100644
--- a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16.pass.cpp
+++ b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16.pass.cpp
@@ -8,7 +8,7 @@
 
 // <codecvt>
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 
 // template <class Elem, unsigned long Maxcode = 0x10ffff,
 //           codecvt_mode Mode = (codecvt_mode)0>
diff --git a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_always_noconv.pass.cpp b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_always_noconv.pass.cpp
index ecf9b670895d3dd..4fd3a5bb484f833 100644
--- a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_always_noconv.pass.cpp
+++ b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_always_noconv.pass.cpp
@@ -8,7 +8,7 @@
 
 // <codecvt>
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 
 // template <class Elem, unsigned long Maxcode = 0x10ffff,
 //           codecvt_mode Mode = (codecvt_mode)0>
diff --git a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_encoding.pass.cpp b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_encoding.pass.cpp
index 9e16daa221fb080..d24983bd88ca806 100644
--- a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_encoding.pass.cpp
+++ b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_encoding.pass.cpp
@@ -8,7 +8,7 @@
 
 // <codecvt>
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 
 // template <class Elem, unsigned long Maxcode = 0x10ffff,
 //           codecvt_mode Mode = (codecvt_mode)0>
diff --git a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_in.pass.cpp b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_in.pass.cpp
index f9dc6f0920355b9..4858543262bdcae 100644
--- a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_in.pass.cpp
+++ b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_in.pass.cpp
@@ -8,7 +8,7 @@
 
 // <codecvt>
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 
 // template <class Elem, unsigned long Maxcode = 0x10ffff,
 //           codecvt_mode Mode = (codecvt_mode)0>
diff --git a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_length.pass.cpp b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_length.pass.cpp
index 68898957599f3d1..1b8efc7bae0d974 100644
--- a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_length.pass.cpp
+++ b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_length.pass.cpp
@@ -8,7 +8,7 @@
 
 // <codecvt>
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 
 // template <class Elem, unsigned long Maxcode = 0x10ffff,
 //           codecvt_mode Mode = (codecvt_mode)0>
diff --git a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_max_length.pass.cpp b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_max_length.pass.cpp
index 3a5f1e972b0d080..3124c47004d1c7a 100644
--- a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_max_length.pass.cpp
+++ b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_max_length.pass.cpp
@@ -8,7 +8,7 @@
 
 // <codecvt>
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 
 // template <class Elem, unsigned long Maxcode = 0x10ffff,
 //           codecvt_mode Mode = (codecvt_mode)0>
diff --git a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_out.pass.cpp b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_out.pass.cpp
index 951e9cb51de7096..11a38ff7207ecda 100644
--- a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_out.pass.cpp
+++ b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_out.pass.cpp
@@ -8,7 +8,7 @@
 
 // <codecvt>
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 
 // template <class Elem, unsigned long Maxcode = 0x10ffff,
 //           codecvt_mode Mode = (codecvt_mode)0>
diff --git a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_unshift.pass.cpp b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_unshift.pass.cpp
index fa6af0a7f3120a0..968770425f59719 100644
--- a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_unshift.pass.cpp
+++ b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf16_unshift.pass.cpp
@@ -8,7 +8,7 @@
 
 // <codecvt>
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 
 // template <class Elem, unsigned long Maxcode = 0x10ffff,
 //           codecvt_mode Mode = (codecvt_mode)0>
diff --git a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8.pass.cpp b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8.pass.cpp
index effd8d5317eadd3..19cbefcadac8259 100644
--- a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8.pass.cpp
+++ b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8.pass.cpp
@@ -8,7 +8,7 @@
 
 // <codecvt>
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 
 // template <class Elem, unsigned long Maxcode = 0x10ffff,
 //           codecvt_mode Mode = (codecvt_mode)0>
diff --git a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_always_noconv.pass.cpp b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_always_noconv.pass.cpp
index f6bb25b41acf0ff..b6b914028d68765 100644
--- a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_always_noconv.pass.cpp
+++ b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_always_noconv.pass.cpp
@@ -8,7 +8,7 @@
 
 // <codecvt>
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 
 // template <class Elem, unsigned long Maxcode = 0x10ffff,
 //           codecvt_mode Mode = (codecvt_mode)0>
diff --git a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_encoding.pass.cpp b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_encoding.pass.cpp
index 25f308170a79b9c..f3a8affdf7d0f67 100644
--- a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_encoding.pass.cpp
+++ b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_encoding.pass.cpp
@@ -8,7 +8,7 @@
 
 // <codecvt>
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 
 // template <class Elem, unsigned long Maxcode = 0x10ffff,
 //           codecvt_mode Mode = (codecvt_mode)0>
diff --git a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_in.pass.cpp b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_in.pass.cpp
index 7f56bc0073ad90e..54f48426a5248ba 100644
--- a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_in.pass.cpp
+++ b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_in.pass.cpp
@@ -8,7 +8,7 @@
 
 // <codecvt>
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 
 // template <class Elem, unsigned long Maxcode = 0x10ffff,
 //           codecvt_mode Mode = (codecvt_mode)0>
diff --git a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_length.pass.cpp b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_length.pass.cpp
index bea2ba3d15768aa..3be8d96a2bad301 100644
--- a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_length.pass.cpp
+++ b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_length.pass.cpp
@@ -8,7 +8,7 @@
 
 // <codecvt>
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 
 // template <class Elem, unsigned long Maxcode = 0x10ffff,
 //           codecvt_mode Mode = (codecvt_mode)0>
diff --git a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_max_length.pass.cpp b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_max_length.pass.cpp
index 41e9e5cc5654ff8..99bb076a4a97bb3 100644
--- a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_max_length.pass.cpp
+++ b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_max_length.pass.cpp
@@ -8,7 +8,7 @@
 
 // <codecvt>
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 
 // template <class Elem, unsigned long Maxcode = 0x10ffff,
 //           codecvt_mode Mode = (codecvt_mode)0>
diff --git a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_out.pass.cpp b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_out.pass.cpp
index b6f872e9068c1cc..e79b65fb265e90d 100644
--- a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_out.pass.cpp
+++ b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_out.pass.cpp
@@ -8,7 +8,7 @@
 
 // <codecvt>
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 
 // template <class Elem, unsigned long Maxcode = 0x10ffff,
 //           codecvt_mode Mode = (codecvt_mode)0>
diff --git a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_unshift.pass.cpp b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_unshift.pass.cpp
index 7bc4d02bf82374e..15bb08ad3c3fef9 100644
--- a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_unshift.pass.cpp
+++ b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_unshift.pass.cpp
@@ -8,7 +8,7 @@
 
 // <codecvt>
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 
 // template <class Elem, unsigned long Maxcode = 0x10ffff,
 //           codecvt_mode Mode = (codecvt_mode)0>
diff --git a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_always_noconv.pass.cpp b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_always_noconv.pass.cpp
index 82b99a38de1a7a2..5cab262d9df73bf 100644
--- a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_always_noconv.pass.cpp
+++ b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_always_noconv.pass.cpp
@@ -8,7 +8,7 @@
 
 // <codecvt>
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 
 // template <class Elem, unsigned long Maxcode = 0x10ffff,
 //           codecvt_mode Mode = (codecvt_mode)0>
diff --git a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_encoding.pass.cpp b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_encoding.pass.cpp
index 2bb6f7b9ccc0095..6f0969b1ae534d5 100644
--- a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_encoding.pass.cpp
+++ b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_encoding.pass.cpp
@@ -8,7 +8,7 @@
 
 // <codecvt>
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 
 // template <class Elem, unsigned long Maxcode = 0x10ffff,
 //           codecvt_mode Mode = (codecvt_mode)0>
diff --git a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_in.pass.cpp b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_in.pass.cpp
index 0cf6290418daa14..e7ca4eede063d08 100644
--- a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_in.pass.cpp
+++ b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_in.pass.cpp
@@ -8,7 +8,7 @@
 
 // <codecvt>
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 
 // template <class Elem, unsigned long Maxcode = 0x10ffff,
 //           codecvt_mode Mode = (codecvt_mode)0>
diff --git a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_length.pass.cpp b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_length.pass.cpp
index 3fcf8e5b2dc522d..2f2b3b599c07dce 100644
--- a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_length.pass.cpp
+++ b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_length.pass.cpp
@@ -8,7 +8,7 @@
 
 // <codecvt>
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 
 // template <class Elem, unsigned long Maxcode = 0x10ffff,
 //           codecvt_mode Mode = (codecvt_mode)0>
diff --git a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_max_length.pass.cpp b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_max_length.pass.cpp
index 06503cbd725af05..1a9b7ffd03dc652 100644
--- a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_max_length.pass.cpp
+++ b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_max_length.pass.cpp
@@ -8,7 +8,7 @@
 
 // <codecvt>
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 
 // template <class Elem, unsigned long Maxcode = 0x10ffff,
 //           codecvt_mode Mode = (codecvt_mode)0>
diff --git a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_out.pass.cpp b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_out.pass.cpp
index 006d9839689f8e6..c1c971c606a2429 100644
--- a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_out.pass.cpp
+++ b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_out.pass.cpp
@@ -8,7 +8,7 @@
 
 // <codecvt>
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 
 // template <class Elem, unsigned long Maxcode = 0x10ffff,
 //           codecvt_mode Mode = (codecvt_mode)0>
diff --git a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_unshift.pass.cpp b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_unshift.pass.cpp
index f1b01ee05633f81..a161ee5ab7f3d8e 100644
--- a/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_unshift.pass.cpp
+++ b/libcxx/test/std/localization/locale.stdcvt/codecvt_utf8_utf16_unshift.pass.cpp
@@ -8,7 +8,7 @@
 
 // <codecvt>
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS, -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
 
 // template <class Elem, unsigned long Maxcode = 0x10ffff,
 //           codecvt_mode Mode = (codecvt_mode)0>
diff --git a/libcxx/test/std/localization/locales/locale.convenience/conversions/conversions.buffer/ctor.pass.cpp b/libcxx/test/std/localization/locales/locale.convenience/conversions/conversions.buffer/ctor.pass.cpp
index e99a85a87398117..f8e6a22373da487 100644
--- a/libcxx/test/std/localization/locales/locale.convenience/conversions/conversions.buffer/ctor.pass.cpp
+++ b/libc...
[truncated]

@philnik777
Copy link
Contributor

I think we should rather go the other way around and make the flag space-separated. Having it comma separated seems like a poor UI design to me, since every compiler I'm aware of assumes a space-separated list of arguments.

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.

TIL! Thanks for fixing these! I found 4 more in test/std, they all seem to be -Xclang -verify-ignore-unexpected=error. There are several more in libcxx. Feel free to fix these too, or leave a message, then I'll create a followup PR.

@StephanTLavavej
Copy link
Member Author

@philnik777

I think we should rather go the other way around and make the flag space-separated. Having it comma separated seems like a poor UI design to me, since every compiler I'm aware of assumes a space-separated list of arguments.

Space-separated seems like a good idea to me too - I didn't try that because it would be a large change to the test harness, and would involve changing a small amount of existing usage that's already using commas.

@mordante

TIL! Thanks for fixing these! I found 4 more in test/std, they all seem to be -Xclang -verify-ignore-unexpected=error. There are several more in libcxx. Feel free to fix these too, or leave a message, then I'll create a followup PR.

You're welcome! If you could create a followup PR that would be great - I'm not equipped to properly test changes to the -Xclang options or anything in libcxx/test/libcxx.

@mordante
Copy link
Member

TIL! Thanks for fixing these! I found 4 more in test/std, they all seem to be -Xclang -verify-ignore-unexpected=error. There are several more in libcxx. Feel free to fix these too, or leave a message, then I'll create a followup PR.

You're welcome! If you could create a followup PR that would be great

No problem, I'll do it after this patch has landed.

  • I'm not equipped to properly test changes to the -Xclang options or anything in libcxx/test/libcxx.

That's why we have a CI ;-)

@ldionne
Copy link
Member

ldionne commented Nov 27, 2023

As a matter of process, I'd rather we fix all of the occurrences in this PR. We have CI in place that makes this easy, and maintainers can even push to this PR directly.

I also concur with Nikolas here, I think we should make it space-delimited instead.

@StephanTLavavej StephanTLavavej changed the title [libc++][test] ADDITIONAL_COMPILE_FLAGS should be a comma-separated list [libc++][test] ADDITIONAL_COMPILE_FLAGS should be a space-separated list Nov 28, 2023
Copy link

github-actions bot commented Nov 28, 2023

✅ With the latest revision this PR passed the Python code formatter.

@StephanTLavavej
Copy link
Member Author

@ldionne @mordante @philnik777 I've reverted the original approach, then implemented space-separated parsing and changed all occurrences of commas in the codebase.

Python interpreter example:

```
>>> line = " -one -two -three"
>>> [s.strip() for s in line.split(" ")]
['', '-one', '-two', '-three']
>>> [s.strip() for s in line.split(" ") if s.strip() != ""]
['-one', '-two', '-three']
```
Copy link
Contributor

@philnik777 philnik777 left a comment

Choose a reason for hiding this comment

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

LGTM, but Louis is a lot more knowledgable in his area, so I'd wait for his review.

llvm/utils/lit/lit/TestRunner.py Show resolved Hide resolved
@mordante
Copy link
Member

We have CI in place that makes this easy, and maintainers can even push to this PR directly.
Good point I forgot about that, still not entirely used to GitHub.

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

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

This is nice, thanks a lot for implementing this approach!

LGTM with a tiny test coverage suggestion.

@StephanTLavavej
Copy link
Member Author

The CI has an unrelated failure, I believe due to #73555. I'm going to go ahead and merge this since the checks were previously OK (aside from spurious failures) and I'm 98% sure that the multiple spaces will work given my previous commit to avoid emitting empty strings.

@StephanTLavavej StephanTLavavej merged commit 4e2216e into llvm:main Nov 30, 2023
7 of 10 checks passed
@StephanTLavavej StephanTLavavej deleted the stl-7-oxford-commas branch November 30, 2023 21:55
StephanTLavavej added a commit to StephanTLavavej/llvm-project that referenced this pull request Nov 30, 2023
llvmGH-73541 added lines to
llvm/utils/lit/tests/Inputs/testrunner-custom-parsers/test.txt
so what was previously on line 7 is now on line 12.
StephanTLavavej added a commit that referenced this pull request Nov 30, 2023
My #73541 added lines to
`llvm/utils/lit/tests/Inputs/testrunner-custom-parsers/test.txt` so what
was previously on line 7 is now on line 12.

Before:

https://github.com/llvm/llvm-project/blob/28412d1800e391c5ba8e7607bb15c74b106d581b/llvm/utils/lit/tests/Inputs/testrunner-custom-parsers/test.txt#L7-L8

After:

https://github.com/llvm/llvm-project/blob/6fb7c2d713587a061cd281eda917746750559380/llvm/utils/lit/tests/Inputs/testrunner-custom-parsers/test.txt#L12-L13

This didn't show up in the PR checks, but caused a buildbot failure
after merging, https://lab.llvm.org/buildbot/#/builders/139/builds/54597
:

```
# | ======================================================================
# | FAIL: test_commands (__main__.TestIntegratedTestKeywordParser)
# | ----------------------------------------------------------------------
# | Traceback (most recent call last):
# |   File "/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/utils/lit/tests/unit/TestRunner.py", line 135, in test_commands
# |     self.assertEqual(value[1].command.strip(), "%dbg(MY_RUN: at line 7)  foo  bar")
# | AssertionError: '%dbg(MY_RUN: at line 12)  foo  bar' != '%dbg(MY_RUN: at line 7)  foo  bar'
# | - %dbg(MY_RUN: at line 12)  foo  bar
# | ?                      ^^
# | + %dbg(MY_RUN: at line 7)  foo  bar
# | ?                      ^
```

Apologies for the build break 🙀
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. llvm-lit testing-tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants