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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision as: ldionne.
ldionne added a comment.

My understanding is that this patch doesn't remove any warnings (if so, @philnik please call them out) -- it only introduces new macros to push/pop warnings consistently like we've started doing in the libc++ source code itself. It also seems to remove a few unused pragmas, which I think is fine as a fly-by.

So I'm fine with this, however I would like to wait for D120684 <https://reviews.llvm.org/D120684> to land before we land this one to make sure everything still works.



================
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")
----------------
philnik wrote:
> 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?
I *think* it's because this header is included elsewhere e.g. in the test suite, but I'm speculating. Eric should know for sure, since he wrote it.


================
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
 
----------------
philnik wrote:
> 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?
@EricWF FWIW, our intent is for everyone who uses the test suite to be doing so using Lit. The Lit setup has been made more flexible to make it possible for everyone to use it with the configuration they need. I'm not saying it's perfect, but that's the intent. Otherwise, it's just too difficult to keep the test suite independent of how Lit works.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121552