Skip to content

[libc++][test] Enhance ADDITIONAL_COMPILE_FLAGS to work with MSVC #74974

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

Conversation

StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Dec 10, 2023

Found while running libc++'s tests with MSVC's STL.

The ADDITIONAL_COMPILE_FLAGS machinery is very useful, but also very problematic for MSVC, as it speaks Martian and doesn't understand most of Clang's compiler options. (-Dmeow is mutually intelligible and that's about it.) We've been dealing with this by simply marking anything that uses ADDITIONAL_COMPILE_FLAGS as FAIL or SKIPPED, but that creates significant gaps in test coverage.

Fortunately, ADDITIONAL_COMPILE_FLAGS also supports "features", which can be slightly enhanced to send Clang-compatible and MSVC-compatible options to the right compilers.

  • Add a new feature gcc-style-warnings.
    • The idea is that this should be defined for Apple Clang, vanilla Clang, or GCC, but not MSVC's cl.exe. Then this feature should be used when a test wants to compile with -Wno-meow. This is the only "ongoing maintenance" request here, which should be minimal and easy to fix if any occurrences are forgotten under libcxx/test/std.
    • In libcxx/utils/libcxx/test/features.py, I've implemented this by adding _isAnyClangOrGCC. I've also slightly refactored the logic (no functional changes) by adding _isAnyClang and _isAnyGCC, along the lines of @h-vetinari's suggestion.
  • Also add (gcc-style-warnings) to an inactive ADDITIONAL_COMPILE_FLAGS in libcxx/test/std/iterators/iterator.primitives/iterator.operations/advance.pass.cpp, so it won't be forgotten.
  • Add ADDITIONAL_COMPILE_FLAGS(gcc-style-warnings): -Wno-undefined-inline to range.lazy.split/constraints.compile.pass.cpp.
    • As the comment explains, this is compile-only, so the warning is irrelevant.
  • Add ADDITIONAL_COMPILE_FLAGS(cl-style-warnings).
    • The idea is that this should be defined for MSVC's cl.exe only, so it's defined as the antonym of _isAnyClangOrGCC.
    • Note that the existing msvc feature is completely different - it indicates the presence of the _MSC_VER compiler macro, i.e. any compiler pretending to be MSVC.
      _isMSVC = lambda cfg: "_MSC_VER" in compilerMacros(cfg)
      Feature(name="msvc", when=_isMSVC),
    • @philnik777's naming idea cl-style-warnings makes this distinction clear - it indicates the actual cl.exe compiler driver.
      • Fun fact, nobody knows what CL means anymore. I asked our longest-tenured compiler devs (30+ years!) and their best guess was "Compile and Link". 😹
    • Because MSVC's /wdNNNN isn't self-explanatory, I'm commenting every occurrence with what the warning message is.
    • Some warnings being silenced are sign/truncation warnings that were intentional (e.g. caused by calling STL algorithms with types that cause the STL to perform mixed-sign comparisons), or that were relatively innocuous and would be invasive or verbose to physically fix. I tried to fix as much as I could in [libc++][test] Fix MSVC warnings with static_casts #74962, [libc++][test] Fix more MSVC and Clang warnings #74965, and previous PRs.
    • Other warnings being silenced are counterparts of Clang warnings being silenced (e.g. -Wno-deprecated-volatile).
    • "warning C4197: top-level volatile in cast is ignored" is emitted for T(); when T is volatile atomic<Whatever>, which is innocuous.
    • "warning C4611: interaction between '_setjmp' and C++ object destruction is non-portable" is emitted in setjmp tests. I didn't see any concerning usage.
    • "warning C4583: 'X::cv_': destructor is not implicitly called" was emitted in a test that explicitly destroys x.cv_ so it's fine.
    • There's no expectation that as libc++ tests are added, that ADDITIONAL_COMPILE_FLAGS(cl-style-warnings) ought to be added accordingly. In fact, I'd prefer it if you didn't, allowing us to review such warnings as they appear. (Of course, if a test is copy-paste-modified, it would be reasonable to preserve any warning options that are unaffected by the modifications, that would never be a problem for us.)
  • Add ADDITIONAL_COMPILE_FLAGS(cl-style-warnings) to silence a blizzard of x86 truncation warnings.

@StephanTLavavej StephanTLavavej requested a review from a team as a code owner December 10, 2023 06:17
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Dec 10, 2023
@llvmbot
Copy link
Member

llvmbot commented Dec 10, 2023

@llvm/pr-subscribers-libcxx

Author: Stephan T. Lavavej (StephanTLavavej)

Changes

Found while running libc++'s tests with MSVC's STL.

The ADDITIONAL_COMPILE_FLAGS machinery is very useful, but also very problematic for MSVC, as it speaks Martian and doesn't understand most of Clang's compiler options. (-Dmeow is mutually intelligible and that's about it.) We've been dealing with this by simply marking anything that uses ADDITIONAL_COMPILE_FLAGS as FAIL or SKIPPED, but that creates significant gaps in test coverage.

Fortunately, ADDITIONAL_COMPILE_FLAGS also supports "features", which can be slightly enhanced to send Clang-compatible and MSVC-compatible options to the right compilers.

  • Use the verify-support feature.
  • Add a new feature any-clang-or-gcc.
    • The idea is that this should be defined for Apple Clang, vanilla Clang, or GCC, but not MSVC's cl.exe. Then this feature should be used when a test wants to compile with -Wno-meow. This is the only "ongoing maintenance" request here, which should be minimal and easy to fix if any occurrences are forgotten under libcxx/test/std.
  • Add ADDITIONAL_COMPILE_FLAGS(any-clang-or-gcc): -Wno-undefined-inline to range.lazy.split/constraints.compile.pass.cpp.
    • As the comment explains, this is compile-only, so the warning is irrelevant.
  • Add ADDITIONAL_COMPILE_FLAGS(msvc-cl-exe).
    • The idea is that this should be defined for MSVC's cl.exe only. I'm adding such a definition to the MSVC STL test harness, which is why it doesn't appear here.
    • Note that the existing msvc feature is completely different - it indicates the presence of the _MSC_VER compiler macro, i.e. any compiler pretending to be MSVC.
      _isMSVC = lambda cfg: "_MSC_VER" in compilerMacros(cfg)
      Feature(name="msvc", when=_isMSVC),
    • I've named msvc-cl-exe to make this distinction clear - it indicates the actual cl.exe compiler driver.
      • Fun fact, nobody knows what CL means anymore. I asked our longest-tenured compiler devs (30+ years!) and their best guess was "Compile and Link". 😹
    • Because MSVC's /wdNNNN isn't self-explanatory, I'm commenting every occurrence with what the warning message is.
    • Some warnings being silenced are sign/truncation warnings that were intentional (e.g. caused by calling STL algorithms with types that cause the STL to perform mixed-sign comparisons), or that were relatively innocuous and would be invasive or verbose to physically fix. I tried to fix as much as I could in #74962, #74965, and previous PRs.
    • Other warnings being silenced are counterparts of Clang warnings being silenced (e.g. -Wno-deprecated-volatile).
    • "warning C4197: top-level volatile in cast is ignored" is emitted for T(); when T is volatile atomic&lt;Whatever&gt;, which is innocuous.
    • "warning C4611: interaction between '_setjmp' and C++ object destruction is non-portable" is emitted in setjmp tests. I didn't see any concerning usage.
    • "warning C4583: 'X::cv_': destructor is not implicitly called" was emitted in a test that explicitly destroys x.cv_ so it's fine.
    • There's no expectation that as libc++ tests are added, that ADDITIONAL_COMPILE_FLAGS(msvc-cl-exe) ought to be added accordingly. In fact, I'd prefer it if you didn't, allowing us to review such warnings as they appear. (Of course, if a test is copy-paste-modified, it would be reasonable to preserve any warning options that are unaffected by the modifications, that would never be a problem for us.)
  • Add ADDITIONAL_COMPILE_FLAGS(msvc-cl-exe) to silence a blizzard of x86 truncation warnings.

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

36 Files Affected:

  • (modified) libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges.replace.pass.cpp (+3)
  • (modified) libcxx/test/std/algorithms/alg.nonmodifying/alg.equal/equal.pass.cpp (+6-1)
  • (modified) libcxx/test/std/algorithms/alg.nonmodifying/alg.find/find.pass.cpp (+3-1)
  • (modified) libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find.pass.cpp (+4-1)
  • (modified) libcxx/test/std/atomics/atomics.types.generic/general.compile.pass.cpp (+3)
  • (modified) libcxx/test/std/atomics/atomics.types.generic/pointer.compile.pass.cpp (+3)
  • (modified) libcxx/test/std/atomics/atomics.types.generic/trivially_copyable.verify.cpp (+1-1)
  • (modified) libcxx/test/std/concepts/concepts.lang/concept.default.init/default_initializable.compile.pass.cpp (+1-1)
  • (modified) libcxx/test/std/containers/associative/map/map.modifiers/insert_range.pass.cpp (+1-1)
  • (modified) libcxx/test/std/containers/associative/multimap/multimap.modifiers/insert_range.pass.cpp (+1-1)
  • (modified) libcxx/test/std/containers/associative/multiset/insert_range.pass.cpp (+1-1)
  • (modified) libcxx/test/std/containers/associative/set/insert_range.pass.cpp (+1-1)
  • (modified) libcxx/test/std/containers/sequences/array/array.tuple/get.verify.cpp (+1-1)
  • (modified) libcxx/test/std/containers/unord/unord.map/unord.map.modifiers/insert_range.pass.cpp (+1-1)
  • (modified) libcxx/test/std/containers/unord/unord.multimap/unord.multimap.modifiers/insert_range.pass.cpp (+1-1)
  • (modified) libcxx/test/std/containers/unord/unord.multiset/insert_range.pass.cpp (+1-1)
  • (modified) libcxx/test/std/containers/unord/unord.set/insert_range.pass.cpp (+1-1)
  • (modified) libcxx/test/std/containers/views/mdspan/layout_stride/deduction.pass.cpp (+1-1)
  • (modified) libcxx/test/std/containers/views/mdspan/mdspan/conversion.pass.cpp (+3)
  • (modified) libcxx/test/std/containers/views/views.span/span.sub/subspan.verify.cpp (+1-1)
  • (modified) libcxx/test/std/depr/depr.c.headers/setjmp_h.compile.pass.cpp (+3)
  • (modified) libcxx/test/std/depr/depr.numeric.limits.has.denorm/deprecated.verify.cpp (+1-1)
  • (modified) libcxx/test/std/input.output/file.streams/fstreams/ifstream.members/buffered_reads.pass.cpp (+3)
  • (modified) libcxx/test/std/input.output/file.streams/fstreams/ofstream.members/buffered_writes.pass.cpp (+3)
  • (modified) libcxx/test/std/language.support/support.runtime/csetjmp.pass.cpp (+3)
  • (modified) libcxx/test/std/ranges/range.adaptors/range.lazy.split/constraints.compile.pass.cpp (+3)
  • (modified) libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp (+3)
  • (modified) libcxx/test/std/strings/basic.string/string.cons/from_range_deduction.pass.cpp (+1-1)
  • (modified) libcxx/test/std/thread/thread.condition/notify_all_at_thread_exit_lwg3343.pass.cpp (+3)
  • (modified) libcxx/test/std/thread/thread.jthread/assign.move.pass.cpp (+1-1)
  • (modified) libcxx/test/std/utilities/any/any.nonmembers/any.cast/const_correctness.verify.cpp (+1-1)
  • (modified) libcxx/test/std/utilities/any/any.nonmembers/any.cast/not_copy_constructible.verify.cpp (+1-1)
  • (modified) libcxx/test/std/utilities/meta/meta.unary/dependent_return_type.compile.pass.cpp (+3-1)
  • (modified) libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.asgn/move.pass.cpp (+1-1)
  • (modified) libcxx/test/std/utilities/variant/variant.variant/implicit_ctad.pass.cpp (+1-1)
  • (modified) libcxx/utils/libcxx/test/features.py (+3)
diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges.replace.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges.replace.pass.cpp
index ab2f705bafe9d0..32da9345c97e15 100644
--- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges.replace.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges.replace.pass.cpp
@@ -6,6 +6,9 @@
 //
 //===----------------------------------------------------------------------===//
 
+// MSVC warning C4244: 'argument': conversion from 'const _Ty2' to 'T', possible loss of data
+// ADDITIONAL_COMPILE_FLAGS(msvc-cl-exe): /wd4244
+
 // <algorithm>
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17
diff --git a/libcxx/test/std/algorithms/alg.nonmodifying/alg.equal/equal.pass.cpp b/libcxx/test/std/algorithms/alg.nonmodifying/alg.equal/equal.pass.cpp
index e28cbe2a08de4a..93ca4827a6e54d 100644
--- a/libcxx/test/std/algorithms/alg.nonmodifying/alg.equal/equal.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.nonmodifying/alg.equal/equal.pass.cpp
@@ -19,7 +19,12 @@
 //   equal(Iter1 first1, Iter1 last1, Iter2 first2, Iter2 last2);
 
 // We test the cartesian product, so we sometimes compare differently signed types
-// ADDITIONAL_COMPILE_FLAGS: -Wno-sign-compare
+// ADDITIONAL_COMPILE_FLAGS(any-clang-or-gcc): -Wno-sign-compare
+// MSVC warning C4242: 'argument': conversion from 'int' to 'const _Ty', possible loss of data
+// MSVC warning C4244: 'argument': conversion from 'wchar_t' to 'const _Ty', possible loss of data
+// MSVC warning C4310: cast truncates constant value
+// MSVC warning C4389: '==': signed/unsigned mismatch
+// ADDITIONAL_COMPILE_FLAGS(msvc-cl-exe): /wd4242 /wd4244 /wd4310 /wd4389
 
 #include <algorithm>
 #include <cassert>
diff --git a/libcxx/test/std/algorithms/alg.nonmodifying/alg.find/find.pass.cpp b/libcxx/test/std/algorithms/alg.nonmodifying/alg.find/find.pass.cpp
index b55a852c10cafa..64fcf9976f772c 100644
--- a/libcxx/test/std/algorithms/alg.nonmodifying/alg.find/find.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.nonmodifying/alg.find/find.pass.cpp
@@ -6,7 +6,9 @@
 //
 //===----------------------------------------------------------------------===//
 
-// ADDITIONAL_COMPILE_FLAGS: -Wno-sign-compare
+// ADDITIONAL_COMPILE_FLAGS(any-clang-or-gcc): -Wno-sign-compare
+// MSVC warning C4389: '==': signed/unsigned mismatch
+// ADDITIONAL_COMPILE_FLAGS(msvc-cl-exe): /wd4389
 
 // <algorithm>
 
diff --git a/libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find.pass.cpp b/libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find.pass.cpp
index 22f938f73ae078..3b60495b6667d2 100644
--- a/libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find.pass.cpp
@@ -10,7 +10,10 @@
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17
 
-// ADDITIONAL_COMPILE_FLAGS: -Wno-sign-compare
+// ADDITIONAL_COMPILE_FLAGS(any-clang-or-gcc): -Wno-sign-compare
+// MSVC warning C4242: 'argument': conversion from 'const _Ty' to 'ElementT', possible loss of data
+// MSVC warning C4244: 'argument': conversion from 'const _Ty' to 'ElementT', possible loss of data
+// ADDITIONAL_COMPILE_FLAGS(msvc-cl-exe): /wd4242 /wd4244
 
 // template<input_iterator I, sentinel_for<I> S, class T, class Proj = identity>
 //   requires indirect_binary_predicate<ranges::equal_to, projected<I, Proj>, const T*>
diff --git a/libcxx/test/std/atomics/atomics.types.generic/general.compile.pass.cpp b/libcxx/test/std/atomics/atomics.types.generic/general.compile.pass.cpp
index 9f2a28256184ce..c43c2e5ac95372 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/general.compile.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/general.compile.pass.cpp
@@ -6,6 +6,9 @@
 //
 //===----------------------------------------------------------------------===//
 
+// MSVC warning C4197: 'volatile std::atomic<operator_hijacker>': top-level volatile in cast is ignored
+// ADDITIONAL_COMPILE_FLAGS(msvc-cl-exe): /wd4197
+
 // UNSUPPORTED: c++03
 
 // XFAIL: availability-synchronization_library-missing
diff --git a/libcxx/test/std/atomics/atomics.types.generic/pointer.compile.pass.cpp b/libcxx/test/std/atomics/atomics.types.generic/pointer.compile.pass.cpp
index 9049beaa9c7897..acc5464e5f690f 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/pointer.compile.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/pointer.compile.pass.cpp
@@ -6,6 +6,9 @@
 //
 //===----------------------------------------------------------------------===//
 
+// MSVC warning C4197: 'volatile std::atomic<operator_hijacker *>': top-level volatile in cast is ignored
+// ADDITIONAL_COMPILE_FLAGS(msvc-cl-exe): /wd4197
+
 // UNSUPPORTED: c++03
 
 // XFAIL: availability-synchronization_library-missing
diff --git a/libcxx/test/std/atomics/atomics.types.generic/trivially_copyable.verify.cpp b/libcxx/test/std/atomics/atomics.types.generic/trivially_copyable.verify.cpp
index 0955707cdcf38f..80e77048ecd858 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/trivially_copyable.verify.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/trivially_copyable.verify.cpp
@@ -15,7 +15,7 @@
 // is not trivially copyable, however Clang will sometimes emit additional
 // errors while trying to instantiate the rest of std::atomic<T>.
 // We silence those to make the test more robust.
-// ADDITIONAL_COMPILE_FLAGS: -Xclang -verify-ignore-unexpected=error
+// ADDITIONAL_COMPILE_FLAGS(verify-support): -Xclang -verify-ignore-unexpected=error
 
 #include <atomic>
 
diff --git a/libcxx/test/std/concepts/concepts.lang/concept.default.init/default_initializable.compile.pass.cpp b/libcxx/test/std/concepts/concepts.lang/concept.default.init/default_initializable.compile.pass.cpp
index 4921a48bcccc17..87c3ed0f8803d9 100644
--- a/libcxx/test/std/concepts/concepts.lang/concept.default.init/default_initializable.compile.pass.cpp
+++ b/libcxx/test/std/concepts/concepts.lang/concept.default.init/default_initializable.compile.pass.cpp
@@ -10,7 +10,7 @@
 
 // We voluntarily use std::default_initializable on types that have redundant
 // or ignored cv-qualifiers -- don't warn about it.
-// ADDITIONAL_COMPILE_FLAGS: -Wno-ignored-qualifiers
+// ADDITIONAL_COMPILE_FLAGS(any-clang-or-gcc): -Wno-ignored-qualifiers
 
 // template<class T>
 //     concept default_initializable = constructible_from<T> &&
diff --git a/libcxx/test/std/containers/associative/map/map.modifiers/insert_range.pass.cpp b/libcxx/test/std/containers/associative/map/map.modifiers/insert_range.pass.cpp
index 1d7fe7193facf4..6f1c6388408539 100644
--- a/libcxx/test/std/containers/associative/map/map.modifiers/insert_range.pass.cpp
+++ b/libcxx/test/std/containers/associative/map/map.modifiers/insert_range.pass.cpp
@@ -8,7 +8,7 @@
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
 // Some fields in the test case variables are deliberately not explicitly initialized, this silences a warning on GCC.
-// ADDITIONAL_COMPILE_FLAGS: -Wno-missing-field-initializers
+// ADDITIONAL_COMPILE_FLAGS(any-clang-or-gcc): -Wno-missing-field-initializers
 
 // <map>
 
diff --git a/libcxx/test/std/containers/associative/multimap/multimap.modifiers/insert_range.pass.cpp b/libcxx/test/std/containers/associative/multimap/multimap.modifiers/insert_range.pass.cpp
index c7c05a896bc9f9..c9d259d03efe88 100644
--- a/libcxx/test/std/containers/associative/multimap/multimap.modifiers/insert_range.pass.cpp
+++ b/libcxx/test/std/containers/associative/multimap/multimap.modifiers/insert_range.pass.cpp
@@ -8,7 +8,7 @@
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
 // Some fields in the test case variables are deliberately not explicitly initialized, this silences a warning on GCC.
-// ADDITIONAL_COMPILE_FLAGS: -Wno-missing-field-initializers
+// ADDITIONAL_COMPILE_FLAGS(any-clang-or-gcc): -Wno-missing-field-initializers
 
 // <map>
 
diff --git a/libcxx/test/std/containers/associative/multiset/insert_range.pass.cpp b/libcxx/test/std/containers/associative/multiset/insert_range.pass.cpp
index 9dd85eea47c290..eb125f1520ec1b 100644
--- a/libcxx/test/std/containers/associative/multiset/insert_range.pass.cpp
+++ b/libcxx/test/std/containers/associative/multiset/insert_range.pass.cpp
@@ -8,7 +8,7 @@
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
 // Some fields in the test case variables are deliberately not explicitly initialized, this silences a warning on GCC.
-// ADDITIONAL_COMPILE_FLAGS: -Wno-missing-field-initializers
+// ADDITIONAL_COMPILE_FLAGS(any-clang-or-gcc): -Wno-missing-field-initializers
 
 // <set>
 
diff --git a/libcxx/test/std/containers/associative/set/insert_range.pass.cpp b/libcxx/test/std/containers/associative/set/insert_range.pass.cpp
index 1956fc6bd7f3ea..1ae7f7d5cd16e8 100644
--- a/libcxx/test/std/containers/associative/set/insert_range.pass.cpp
+++ b/libcxx/test/std/containers/associative/set/insert_range.pass.cpp
@@ -8,7 +8,7 @@
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
 // Some fields in the test case variables are deliberately not explicitly initialized, this silences a warning on GCC.
-// ADDITIONAL_COMPILE_FLAGS: -Wno-missing-field-initializers
+// ADDITIONAL_COMPILE_FLAGS(any-clang-or-gcc): -Wno-missing-field-initializers
 
 // <set>
 
diff --git a/libcxx/test/std/containers/sequences/array/array.tuple/get.verify.cpp b/libcxx/test/std/containers/sequences/array/array.tuple/get.verify.cpp
index 9052b4359f6b0a..c93ea75c221dac 100644
--- a/libcxx/test/std/containers/sequences/array/array.tuple/get.verify.cpp
+++ b/libcxx/test/std/containers/sequences/array/array.tuple/get.verify.cpp
@@ -11,7 +11,7 @@
 // template <size_t I, class T, size_t N> T& get(array<T, N>& a);
 
 // Prevent -Warray-bounds from issuing a diagnostic when testing with clang verify.
-// ADDITIONAL_COMPILE_FLAGS: -Wno-array-bounds
+// ADDITIONAL_COMPILE_FLAGS(any-clang-or-gcc): -Wno-array-bounds
 
 #include <array>
 #include <cassert>
diff --git a/libcxx/test/std/containers/unord/unord.map/unord.map.modifiers/insert_range.pass.cpp b/libcxx/test/std/containers/unord/unord.map/unord.map.modifiers/insert_range.pass.cpp
index 8b004336f68cde..f524273bdda03d 100644
--- a/libcxx/test/std/containers/unord/unord.map/unord.map.modifiers/insert_range.pass.cpp
+++ b/libcxx/test/std/containers/unord/unord.map/unord.map.modifiers/insert_range.pass.cpp
@@ -8,7 +8,7 @@
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
 // Some fields in the test case variables are deliberately not explicitly initialized, this silences a warning on GCC.
-// ADDITIONAL_COMPILE_FLAGS: -Wno-missing-field-initializers
+// ADDITIONAL_COMPILE_FLAGS(any-clang-or-gcc): -Wno-missing-field-initializers
 
 // <unordered_map>
 
diff --git a/libcxx/test/std/containers/unord/unord.multimap/unord.multimap.modifiers/insert_range.pass.cpp b/libcxx/test/std/containers/unord/unord.multimap/unord.multimap.modifiers/insert_range.pass.cpp
index fcde119f487030..7219225ff16d95 100644
--- a/libcxx/test/std/containers/unord/unord.multimap/unord.multimap.modifiers/insert_range.pass.cpp
+++ b/libcxx/test/std/containers/unord/unord.multimap/unord.multimap.modifiers/insert_range.pass.cpp
@@ -8,7 +8,7 @@
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
 // Some fields in the test case variables are deliberately not explicitly initialized, this silences a warning on GCC.
-// ADDITIONAL_COMPILE_FLAGS: -Wno-missing-field-initializers
+// ADDITIONAL_COMPILE_FLAGS(any-clang-or-gcc): -Wno-missing-field-initializers
 
 // <unordered_map>
 
diff --git a/libcxx/test/std/containers/unord/unord.multiset/insert_range.pass.cpp b/libcxx/test/std/containers/unord/unord.multiset/insert_range.pass.cpp
index 73ac4cf071e1bb..119091c007cb03 100644
--- a/libcxx/test/std/containers/unord/unord.multiset/insert_range.pass.cpp
+++ b/libcxx/test/std/containers/unord/unord.multiset/insert_range.pass.cpp
@@ -8,7 +8,7 @@
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
 // Some fields in the test case variables are deliberately not explicitly initialized, this silences a warning on GCC.
-// ADDITIONAL_COMPILE_FLAGS: -Wno-missing-field-initializers
+// ADDITIONAL_COMPILE_FLAGS(any-clang-or-gcc): -Wno-missing-field-initializers
 
 // <set>
 
diff --git a/libcxx/test/std/containers/unord/unord.set/insert_range.pass.cpp b/libcxx/test/std/containers/unord/unord.set/insert_range.pass.cpp
index c6306a28b7adb5..d9503808c7f2cb 100644
--- a/libcxx/test/std/containers/unord/unord.set/insert_range.pass.cpp
+++ b/libcxx/test/std/containers/unord/unord.set/insert_range.pass.cpp
@@ -8,7 +8,7 @@
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
 // Some fields in the test case variables are deliberately not explicitly initialized, this silences a warning on GCC.
-// ADDITIONAL_COMPILE_FLAGS: -Wno-missing-field-initializers
+// ADDITIONAL_COMPILE_FLAGS(any-clang-or-gcc): -Wno-missing-field-initializers
 
 // <set>
 
diff --git a/libcxx/test/std/containers/views/mdspan/layout_stride/deduction.pass.cpp b/libcxx/test/std/containers/views/mdspan/layout_stride/deduction.pass.cpp
index 0e0a079b598bc0..5384f37fd40c78 100644
--- a/libcxx/test/std/containers/views/mdspan/layout_stride/deduction.pass.cpp
+++ b/libcxx/test/std/containers/views/mdspan/layout_stride/deduction.pass.cpp
@@ -6,7 +6,7 @@
 //
 //===----------------------------------------------------------------------===//
 // UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
-// ADDITIONAL_COMPILE_FLAGS: -Wno-ctad-maybe-unsupported
+// ADDITIONAL_COMPILE_FLAGS(any-clang-or-gcc): -Wno-ctad-maybe-unsupported
 
 // <mdspan>
 
diff --git a/libcxx/test/std/containers/views/mdspan/mdspan/conversion.pass.cpp b/libcxx/test/std/containers/views/mdspan/mdspan/conversion.pass.cpp
index abb647e960c34e..99d786d1d33f5b 100644
--- a/libcxx/test/std/containers/views/mdspan/mdspan/conversion.pass.cpp
+++ b/libcxx/test/std/containers/views/mdspan/mdspan/conversion.pass.cpp
@@ -7,6 +7,9 @@
 //===----------------------------------------------------------------------===//
 // UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
 
+// MSVC warning C4244: 'initializing': conversion from '_Ty' to '_Ty', possible loss of data
+// ADDITIONAL_COMPILE_FLAGS(msvc-cl-exe): /wd4244
+
 // <mdspan>
 
 // template<class OtherElementType, class OtherExtents,
diff --git a/libcxx/test/std/containers/views/views.span/span.sub/subspan.verify.cpp b/libcxx/test/std/containers/views/views.span/span.sub/subspan.verify.cpp
index d6bc21197098a4..eb881707b1e648 100644
--- a/libcxx/test/std/containers/views/views.span/span.sub/subspan.verify.cpp
+++ b/libcxx/test/std/containers/views/views.span/span.sub/subspan.verify.cpp
@@ -9,7 +9,7 @@
 
 // This test also generates spurious warnings when instantiating std::span
 // with a very large extent (like size_t(-2)) -- silence those.
-// ADDITIONAL_COMPILE_FLAGS: -Xclang -verify-ignore-unexpected=warning
+// ADDITIONAL_COMPILE_FLAGS(verify-support): -Xclang -verify-ignore-unexpected=warning
 
 // <span>
 
diff --git a/libcxx/test/std/depr/depr.c.headers/setjmp_h.compile.pass.cpp b/libcxx/test/std/depr/depr.c.headers/setjmp_h.compile.pass.cpp
index eaaeecbeb70ec8..feeefc1194e85a 100644
--- a/libcxx/test/std/depr/depr.c.headers/setjmp_h.compile.pass.cpp
+++ b/libcxx/test/std/depr/depr.c.headers/setjmp_h.compile.pass.cpp
@@ -6,6 +6,9 @@
 //
 //===----------------------------------------------------------------------===//
 
+// MSVC warning C4611: interaction between '_setjmp' and C++ object destruction is non-portable
+// ADDITIONAL_COMPILE_FLAGS(msvc-cl-exe): /wd4611
+
 // test <setjmp.h>
 //
 // Even though <setjmp.h> is not provided by libc++, we still test that
diff --git a/libcxx/test/std/depr/depr.numeric.limits.has.denorm/deprecated.verify.cpp b/libcxx/test/std/depr/depr.numeric.limits.has.denorm/deprecated.verify.cpp
index 98d49de80a58cf..9aa0d0b21d1e0a 100644
--- a/libcxx/test/std/depr/depr.numeric.limits.has.denorm/deprecated.verify.cpp
+++ b/libcxx/test/std/depr/depr.numeric.limits.has.denorm/deprecated.verify.cpp
@@ -8,7 +8,7 @@
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
 
-// ADDITIONAL_COMPILE_FLAGS: -Wno-unused-value
+// ADDITIONAL_COMPILE_FLAGS(any-clang-or-gcc): -Wno-unused-value
 
 #include <limits>
 
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 d57b7c20a2da27..e6a6b650800b6b 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
@@ -7,6 +7,9 @@
 //===----------------------------------------------------------------------===//
 
 // ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// MSVC warning C4242: '+=': conversion from 'const _Ty' to 'size_t', possible loss of data
+// MSVC warning C4244: 'argument': conversion from 'std::streamsize' to 'size_t', possible loss of data
+// ADDITIONAL_COMPILE_FLAGS(msvc-cl-exe): /wd4242 /wd4244
 // 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 e7820739505106..697ea7233de494 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
@@ -7,6 +7,9 @@
 //===----------------------------------------------------------------------===//
 
 // ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT
+// MSVC warning C4242: '+=': conversion from 'const _Ty' to 'size_t', possible loss of data
+// MSVC warning C4244: 'argument': conversion from 'std::streamsize' to 'size_t', possible loss of data
+// ADDITIONAL_COMPILE_FLAGS(msvc-cl-exe): /wd4242 /wd4244
 // UNSUPPORTED: c++03
 
 // <fstream>
diff --git a/libcxx/test/std/language.support/support.runtime/csetjmp.pass.cpp b/libcxx/test/std/language.support/support.runtime/csetjmp.pass.cpp
index d6d32c371b9e5c..c95ebcf8c4b9de 100644
--- a/libcxx/test/std/language.support/support.runtime/csetjmp.pass.cpp
+++ b/libcxx/test/std/language.support/support.runtime/csetjmp.pass.cpp
@@ -6,6 +6,9 @@
 //
 //===----------------------------------------------------------------------===//
 
+// MSVC warning C4611: interaction between '_setjmp' and C++ object destruction is non-portable
+// ADDITIONAL_COMPILE_FLAGS(msvc-cl-exe): /wd4611
+
 // test <csetjmp>
 
 #include <csetjmp>
diff --git a/libcxx/test/std/ranges/range.adaptors/range.lazy.split/constraints.compile.pass.cpp b/libcxx/test/std/ranges/range.adaptors/range.lazy.split/constraints.compile.pass.cpp
index a942f439040929..2a772ccb83f811 100644
--- a/libcxx/test/std/ranges/range.adaptors/range.lazy.split/constraints.compile.pass.cpp
+++ b/libcxx/test/std/ranges/range.adaptors/range.lazy.split/constraints.compile.pass.cpp
@@ -8,6 +8,9 @@
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17
 
+// This is a compile-only test, so "inline function is not defined" warnings are irrelevant.
+// ADDITIONAL_COMPILE_FLAGS(any-clang-or-gcc): -Wno-undefined-inline
+
 // template<input_range V, forward_range Pattern>
 //   requires view<V> && view<Pattern> &&
 //            indirectly_comparable<iterator_t<V>, iterator_t<Pattern>, ranges::equal_to> &&
diff --git a/libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp b/libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp
index 03270f25fd92b2..719e6e818b68e0 100644
--- a/libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp
+++ b/libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp
@@ -6,6 +6,9 @@
 //
 //===----------------------------------------------------------------------===//
 
+// MSVC warning C4244: 'argument': conversion from '_Ty' to 'int', possible loss of data
+// ADDITIONAL_COMPILE_FLAGS(msvc-cl-exe): /wd4244
+
 // UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
 
 // template<class C, input_range R, class... Args> requires (!view<C>)
diff --git a/libcxx/test/std/strings/basic.string/string.cons/from_range_deduction.pass.cpp b/libcxx/test/std/strings/basic.string/string.cons/from_rang...
[truncated]

Copy link

github-actions bot commented Dec 10, 2023

:white_check_mark: With the latest revision this PR passed the Python code formatter.

@cpplearner
Copy link
Contributor

IMHO, TEST_CLANG_DIAGNOSTIC_IGNORED/TEST_GCC_DIAGNOSTIC_IGNORED/TEST_MSVC_DIAGNOSTIC_IGNORED (from test_macros.h) should be the preferred way to disable warnings.

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.

I think I would name the features gcc-style-warnings and cl-style-warnings to make it clear what the intended use here is. I don't think we want anybody to start using these flags for anything else.

…cl-style-warnings`.

I've left `_isAnyClangOrGCC` unchanged, since that's what it's physically checking for.
@StephanTLavavej
Copy link
Member Author

I've reverted the verify-support changes, renamed any-clang-or-gcc => gcc-style-warnings and msvc-cl-exe => cl-style-warnings, and updated the PR description.

In the Python script, I've left _isAnyClangOrGCC unchanged, since that's what it's physically checking for. Let me know if you want that renamed too.

@ldionne ldionne self-assigned this Dec 11, 2023
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 makes sense to me and it seems like a reasonable support burden for us. I have a comment, but this looks pretty good to me.

IMHO, TEST_CLANG_DIAGNOSTIC_IGNORED/TEST_GCC_DIAGNOSTIC_IGNORED/TEST_MSVC_DIAGNOSTIC_IGNORED (from test_macros.h) should be the preferred way to disable warnings.

@cpplearner This is an interesting point. We could also decide to go all out and use the macros instead. TBH I am mostly neutral on this, but I'd be curious to hear what motivates your preference.

@StephanTLavavej
Copy link
Member Author

I think I can prepare an alternative PR to use the macros everywhere. It'll take a bit of time, but I've already done the hard work here of figuring out which files need to suppress which MSVC warnings.

@StephanTLavavej
Copy link
Member Author

I've created #75133 and would prefer to merge that PR, since I was able to locally suppress some warnings and remove some weirdness from msvc_stdlib_force_include.h. There are already a fair number of occurrences of using the macros for warning suppression, so it increases codebase consistency.

@StephanTLavavej
Copy link
Member Author

Closing this PR since I want to proceed with the macro approach.

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.

6 participants