[libcxx-commits] [PATCH] D121552: [libc++] Add warning pragma macros in the test suite

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added inline comments.


================
Comment at: libcxx/src/filesystem/filesystem_common.h:47
 
-#if defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wunused-function"
-#endif
+_LIBCPP_DIAGNOSTIC_PUSH
+_LIBCPP_GCC_DIAGNOSTIC_IGNORED("-Wunused-function")
----------------
I would much rather we annotate each function than disable this for the entire file.
It's a useful warning.


================
Comment at: libcxx/src/filesystem/filesystem_common.h:610
 _LIBCPP_END_NAMESPACE_FILESYSTEM
 
+_LIBCPP_DIAGNOSTIC_POP
----------------
Had this always been missing a `pop`?


================
Comment at: libcxx/test/libcxx/minimal_cxx11_configuration.pass.cpp:28
+
+TEST_CLANG_DIAGNOSTIC_IGNORED("-Wc++11-extensions")
+
----------------
There was a reason this went before the type traits includes I suspect.


================
Comment at: libcxx/test/libcxx/utilities/tuple/tuple.tuple/tuple.cnstr/PR20855_tuple_ref_binding_diagnostics.fail.cpp:15
 
-#ifdef __clang__
-#pragma clang diagnostic ignored "-Wdangling-field"
----------------
I suspect this is needed, and the test just wasn't run with warnings enabled. Otherwise something is broken.
That warning is specifically what the test is trying to generate. If it's not being generated, there's something very weird going on.


================
Comment at: libcxx/test/std/containers/sequences/array/array.tuple/get.fail.cpp:14
 // Prevent -Warray-bounds from issuing a diagnostic when testing with clang verify.
-#if defined(__clang__)
-#pragma clang diagnostic ignored "-Warray-bounds"
-#endif
+// ADDITIONAL_COMPILE_FLAGS: -Wno-array-bounds
 
----------------
Don't make this harder to compile for people who don't use LIT..


================
Comment at: libcxx/test/std/utilities/meta/meta.rel/is_convertible_fallback.pass.cpp:26
 #define _LIBCPP_USE_IS_CONVERTIBLE_FALLBACK
 #include "is_convertible.pass.cpp"
----------------
No. The `#define` NEEDS to be the first thing that occurs. Otherwise the entire test may not test anything.


================
Comment at: libcxx/test/std/utilities/meta/meta.unary/meta.unary.prop/is_destructible.pass.cpp:13
 
-// Prevent warning when testing the Abstract test type.
-#if defined(__clang__)
----------------
Again, I don't think this change was actually tested with warnings enabled.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121552/new/

https://reviews.llvm.org/D121552