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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik 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")
----------------
EricWF wrote:
> I would much rather we annotate each function than disable this for the entire file.
> It's a useful warning.
Almost all functions here are unused, because they are all in an anonymous namespace (and some are also declared `static` just to be safe). Shouldn't these functions be declared `inline` and //not// in an anonymous namespace/declared `static`? Or what is the rationale behind doing these very weird things?


================
Comment at: libcxx/src/filesystem/filesystem_common.h:610
 _LIBCPP_END_NAMESPACE_FILESYSTEM
 
+_LIBCPP_DIAGNOSTIC_POP
----------------
EricWF wrote:
> Had this always been missing a `pop`?
No. You removed it for some reason in e3081d5d96fa2940bbec51d978f98384781ea979.


================
Comment at: libcxx/test/libcxx/minimal_cxx11_configuration.pass.cpp:28
+
+TEST_CLANG_DIAGNOSTIC_IGNORED("-Wc++11-extensions")
+
----------------
EricWF wrote:
> There was a reason this went before the type traits includes I suspect.
Actually, I have no idea why I left that in. The warning is explicitly disabled.


================
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
 
----------------
EricWF wrote:
> Don't make this harder to compile for people who don't use LIT..
There are almost 500 uses of `ADDITIONAL_COMPILE_FLAGS` in the libc++ test suite. Are there even people who don't use lit to run the tests? If yes, what is the problem with using lit?


================
Comment at: libcxx/test/std/utilities/meta/meta.const.eval/is_constant_evaluated.pass.cpp:28
 // because it's explicitly testing those cases.
-#if TEST_HAS_WARNING("-Wconstant-evaluated") && defined(__clang__)
-#pragma clang diagnostic ignored "-Wconstant-evaluated"
-#endif
+TEST_CLANG_DIAGNOSTIC_IGNORED("-Wconstant-evaluated")
+TEST_MSVC_DIAGNOSTIC_IGNORED(5063)
----------------
EricWF wrote:
> What version was `-Wconstant-evaluated` added to Clang?
In Clang 10.


================
Comment at: libcxx/test/std/utilities/meta/meta.const.eval/is_constant_evaluated.pass.cpp:36
 {
-#ifdef __cpp_lib_is_constant_evaluated
-#ifdef TEST_COMPILER_MSVC
----------------
EricWF wrote:
> Where did this go? 
All major implementations are supporting `std::is_constant_evaluated()` for two years now (OK, in two days for two years). GCC since version 9, libc++ since version 9 and MSVC since VS 2019 16.5. I see no reason to guard this anymore. We don't guard any other tests with feature-test macros.


================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.rel/three_way.pass.cpp:22
+TEST_CLANG_DIAGNOSTIC_IGNORED("-Wsign-compare")
+TEST_GCC_DIAGNOSTIC_IGNORED("-Wsign-compare")
+TEST_MSVC_DIAGNOSTIC_IGNORED(4242 4244)
----------------
EricWF wrote:
> 
> Clang respects GCC's pragmas. We don't need both here. 
We decided in D119295 that the best way (we could think of) is to fully separate the GCC and Clang warnings, because there are GCC warnings that Clang doesn't recognize and the macro would be kind-of lying if it disabled warnings for clang while being called `_LIBCPP_GCC_DIAGNOSTIC_IGNORED`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121552