[libcxx-commits] [PATCH] D119295: [libc++] Guard warning pragmas

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__config:1445
+     _LIBCPP_PRAGMA(clang diagnostic ignored warning) _LIBCPP_FORCE_SEMICOLON
+#  define _LIBCPP_GCC_DIAGNOSTIC_IGNORED(warning) _LIBCPP_CLANG_DIAGNOSTIC_IGNORED(warning)
+#elif defined(__GNUG__)
----------------
philnik wrote:
> Mordante wrote:
> > I feel this is a bit misleading. This diagnostic is ignored on both GCC and Clang. Clang supports the gcc diagnostics so that works. However I wonder what happens when a GCC only compiler diagnostic is pushed.
> > 
> > Wouldn't it be better to have
> > `_LIBCPP_CLANG_DIAGNOSTIC_IGNORED`, `_LIBCPP_GCC_DIAGNOSTIC_IGNORED`, and `_LIBCPP_COMPILER_DIAGNOSTIC_IGNORED`?
> > Where `_LIBCPP_COMPILER_DIAGNOSTIC_IGNORED` uses `GCC diagnostic ignored warning`.
> > 
> > That name is still wrong when using other compilers, but that feels better than the current naming.
> I also thought about the naming. `_LIBCPP_GCC_DIAGNOSTIC_IGNORED` didn't really feel right. Would `_LIBCPP_CLANG_GCC_DIAGNOSTIC_IGNORED` be better? And then `_LIBCPP_GCC_DIAGNOSTIC_IGNORED` only enabled on GCC.
> I think `_LIBCPP_COMPILER_DIAGNOSTIC_IGNORED` is to general of a name. That sounds to me to much like some magic attribute that //all// compilers have to support (which would actually be great to have). @ldionne do you have any opinions here?
IMO, it would make sense to do as if  Clang and GCC were completely non-overlapping here. In other words, in the code, we'd have:

```
_LIBCPP_DIAGNOSTIC_PUSH();
_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wunused-private-field");
_LIBCPP_GCC_DIAGNOSTIC_IGNORED("-Wunused-private-field");

    // code

_LIBCPP_DIAGNOSTIC_POP();
```

And one of `_LIBCPP_CLANG_DIAGNOSTIC_IGNORED` and `_LIBCPP_GCC_DIAGNOSTIC_IGNORED` would always expand to nothing. That way, we won't be lying about what `_LIBCPP_<compiler>_DIAGNOSTIC_IGNORED` is doing. It's a tiny bit more work when using it (because you might have to use both the macro for Clang and for GCC), but we're still improving on the status quo where we have enclosing `#if`s.



================
Comment at: libcxx/include/__config:1437
 
+#define _LIBCPP_PRAGMA(text) _Pragma(#text)
+#define _LIBCPP_FORCE_SEMICOLON static_assert(true, "")
----------------
I don't feel that `_LIBCPP_PRAGMA` is *that* useful, since we don't actually save any typing below?


================
Comment at: libcxx/include/__config:1440
+
+#if defined(__clang__)
+#  define _LIBCPP_DIAGNOSTIC_PUSH(reason) _LIBCPP_PRAGMA(clang diagnostic push) _LIBCPP_FORCE_SEMICOLON
----------------
We can use `_LIBCPP_COMPILER_CLANG_BASED` here, and `_LIBCPP_COMPILER_GCC` below.


================
Comment at: libcxx/src/locale.cpp:51
 // lots of noise in the build log, but no bugs that I know of.
-#if defined(__clang__)
-#pragma clang diagnostic ignored "-Wsign-conversion"
-#endif
+_LIBCPP_DIAGNOSTIC_PUSH();
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wsign-conversion");
----------------
Do you really need to push/pop here? And also in the other `.cpp` files? It seems to buy us little, since we're disabling those warnings pretty much for the whole file anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119295