[libc++] Guard warning pragmas

[philnik]

Summary

This makes the GCC output even cleaner!

[ldionne]

Can you describe the difference in output?

[philnik]

In D119295#3307933, @ldionne wrote:

Can you describe the difference in output?

This suppresses -Wunknown-pragma. GCC doesn't know #pragma clang *. This way we also don't have to have push and pop warning macros for every compiler we support everywhere.

[Mordante]

libcxx/include/__config

1445

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.

[philnik]

libcxx/include/__config

1445

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?

[philnik]

• Fix CI

[ldionne]

libcxx/include/__config

1437

I don't feel that _LIBCPP_PRAGMA is *that* useful, since we don't actually save any typing below?

1440

We can use _LIBCPP_COMPILER_CLANG_BASED here, and _LIBCPP_COMPILER_GCC below.

1445

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 #ifs.

libcxx/src/locale.cpp

51

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.

[• Quuxplusone]

libcxx/include/__config

1438

Neither _LIBCPP_PUSH_MACROS nor _LIBCPP_BEGIN_NAMESPACE_STD end with the noise characters ();, and I don't think _LIBCPP_DIAGNOSTIC_PUSH or _LIBCPP_DIAGNOSTIC_POP should, either. Especially since you have to go out of your way with this static_assert hack in order to make the (); syntactically valid C++ code.
Just do

#define _LIBCPP_TOSTRING2(w) #w
#define _LIBCPP_DIAGNOSTIC_PUSH _Pragma("clang diagnostic push")
#define _LIBCPP_CLANG_DIAGNOSTIC_IGNORED(w) _Pragma(_LIBCPP_TOSTRING2(clang diagnostic ignored w))
#define _LIBCPP_DIAGNOSTIC_POP _Pragma("clang diagnostic pop")

(Notice that in this branch we know we're on Clang, so we know _Pragma is supported. I initially agreed with Louis that the extra indirection of _LIBCPP_PRAGMA didn't seem to be buying us anything... but then I realized that what it was actually buying us was stringification! Which is important in this case, but _LIBCPP_PRAGMA is the wrong name for it, and in fact <__config> already provides _LIBCPP_TOSTRING2 ready-made.)

[mstorsjo]

D119134 has landed now, which should be included in this one when rebasing - sorry for the conflicting change!

[philnik]

• Address comments

[• Quuxplusone]

libcxx/include/__config

1440

Naming nit: I'd prefer the macro parameter be named w or x or wOption or wo or str or anything that doesn't camouflage itself so well into the phrase "clang diagnostic ignored warning". Note I believe "clang diagnostic warning ..." is also a thing.
(This applies to all 6 instances of warning.)

1450–1451

Nit: Swap these two lines to keep _LIBCPP_GCC_DIAGNOSTIC_IGNORED and _LIBCPP_CLANG_DIAGNOSTIC_IGNORED in a consistent order in all three branches.

libcxx/src/future.cpp

32–35

Please keep the PUSH and POP here. I see this is in a .cpp file, so the setting won't "leak" out into unrelated files the way it would in a .h file; but still, let's be consistent. Mainly for consistency. Hypothetically so that we don't accidentally disable this warning for switch statements that might one day be added further down in this .cpp file.

libcxx/src/hash.cpp

14

I'd like this to be scoped with PUSH and POP too, right around the lines that actually trigger the diagnostic. But what you've got here preserves the current (suboptimal) behavior, and finding out which lines trigger the diagnostic might be a research project, I dunno, so no action required here (but I think it'd be nice to have).

libcxx/src/locale.cpp

51

I commented in future.cpp with the opposite opinion: I think we should stay in the habit of tightly scoping these workarounds, and using them consistently with PUSH and POP. This makes it easier to look at the code and say "yep, that looks right" without having to worry about "well, it looks wrong, but I see it's (currently) in a .cpp file, so actually it's okay" and so on. We've picked a pattern for disabling warnings; let's stick to it.

Lines 5202 and 5348 are particularly weird: If these aren't tightly scoped workarounds for specific lines of code, then why do we wait more than 5000 lines into the file before doing them? Either these lines are meant to apply to the entire file (in which case they should be at the top of the file and/or in the build system), or they were meant to be tightly scoped workarounds and the programmer simply forgot the PUSH and POP. We shouldn't write code that looks like a mistake.

[philnik]

• Address comments

[mstorsjo]

Can you include the new pragmas from 8cffea061afc556332a86fbf80ef5613db994974 / D119134 (libcxx/src/support/win32/locale_win32.cpp and libcxx/src/support/win32/support.cpp) in this cleanup too?

[philnik]

@mstorsjo thanks for the reminder!

• Rebased

[philnik]

• Replace locale_win32.cpp pragmas

[ldionne]

The direction LGTM but I see a couple of easy to fix problems with the current state of the patch, so I'm requesting changes.

libcxx/include/__config

170–177

I don't think this is intended to be part of this diff. This looks like a rebase issue to me.

1440

This should use _LIBCPP_TOSTRING, not _LIBCPP_TOSTRING2 (ditto below).

libcxx/src/locale.cpp

5202

Don't we need to also add _LIBCPP_GCC_DIAGNOSTIC_IGNORED? In the "before" diff, there were branches for both GCC and Clang. But don't add it if it's not necessary!

[philnik]

• Rebased
• Address comments

libcxx/src/hash.cpp

14

I will probably look into making the scope for the diagnostic suppression smaller, but not in this PR.

libcxx/src/locale.cpp

5202

I get no warnings from GCC, so I guess the warning is currently not enable for GCC. We can add the warning suppression again if we decide to enable the warning for GCC. But right now I want GCC to be able to build libc++ with -Werror.

libcxx/include/__config

 # define _LIBCPP_ABI_ENABLE_ADDITIONAL_IOSTREAM_EXPLICIT_INSTANTIATIONS_1
 
 // Define a key function for `bad_function_call` in the library, to centralize
 // its vtable and typeinfo to libc++ rather than having all other libraries
 // using that class define their own copies.
 #  define _LIBCPP_ABI_BAD_FUNCTION_CALL_KEY_FUNCTION
 #endif
 
+#define _LIBCPP_TOSTRING2(x) #x
+#define _LIBCPP_TOSTRING(x) _LIBCPP_TOSTRING2(x)
 #define _LIBCPP_CONCAT1(_LIBCPP_X,_LIBCPP_Y) _LIBCPP_X##_LIBCPP_Y
 #define _LIBCPP_CONCAT(_LIBCPP_X,_LIBCPP_Y) _LIBCPP_CONCAT1(_LIBCPP_X,_LIBCPP_Y)
 
 #ifndef _LIBCPP_ABI_NAMESPACE
 # define _LIBCPP_ABI_NAMESPACE _LIBCPP_CONCAT(__,_LIBCPP_ABI_VERSION)
 #endif
 
 #if __cplusplus < 201103L
 #define _LIBCPP_CXX03_LANG
 #endif
 
 #ifndef __has_attribute
 #define __has_attribute(__x) 0
 #endif

 #endif
 
 #define _LIBCPP_ALWAYS_INLINE __attribute__ ((__always_inline__))
 
 #define _LIBCPP_DISABLE_EXTENSION_WARNING __extension__
 
 #elif defined(_LIBCPP_COMPILER_MSVC)
 
-#define _LIBCPP_TOSTRING2(x) #x
-#define _LIBCPP_TOSTRING(x) _LIBCPP_TOSTRING2(x)
 #define _LIBCPP_WARNING(x) __pragma(message(__FILE__ "(" _LIBCPP_TOSTRING(__LINE__) ") : warning note: " x))
 
 #if _MSC_VER < 1900
 #error "MSVC versions prior to Visual Studio 2015 are not supported"
 #endif
 
 #define __alignof__ __alignof
 #define _LIBCPP_NORETURN __declspec(noreturn)
#if defined(__GNUC__) || defined(__clang__)
   // The attribute uses 1-based indices for ordinary and static member functions.
   // The attribute uses 2-based indices for non-static member functions.
 # define _LIBCPP_ATTRIBUTE_FORMAT(archetype, format_string_index, first_format_arg_index) \
     __attribute__((__format__(archetype, format_string_index, first_format_arg_index)))
 #else
 # define _LIBCPP_ATTRIBUTE_FORMAT(archetype, format_string_index, first_format_arg_index) /* nothing */
 #endif
 
+#ifdef _LIBCPP_COMPILER_CLANG_BASED
+#  define _LIBCPP_DIAGNOSTIC_PUSH _Pragma("clang diagnostic push")
+#  define _LIBCPP_DIAGNOSTIC_POP _Pragma("clang diagnostic pop")
+#  define _LIBCPP_CLANG_DIAGNOSTIC_IGNORED(str) _Pragma(_LIBCPP_TOSTRING2(clang diagnostic ignored str))
+#  define _LIBCPP_GCC_DIAGNOSTIC_IGNORED(str)
+#elif defined(_LIBCPP_COMPILER_GCC)
+#  define _LIBCPP_DIAGNOSTIC_PUSH _Pragma("GCC diagnostic push")
+#  define _LIBCPP_DIAGNOSTIC_POP _Pragma("GCC diagnostic pop")
+#  define _LIBCPP_CLANG_DIAGNOSTIC_IGNORED(str)
+#  define _LIBCPP_GCC_DIAGNOSTIC_IGNORED(str) _Pragma(_LIBCPP_TOSTRING2(GCC diagnostic ignored str))
+#else
+#  define _LIBCPP_DIAGNOSTIC_PUSH
+#  define _LIBCPP_DIAGNOSTIC_POP
+#  define _LIBCPP_CLANG_DIAGNOSTIC_IGNORED(str)
+#  define _LIBCPP_GCC_DIAGNOSTIC_IGNORED(str)
+#endif
+
 #endif // __cplusplus
 
 #endif // _LIBCPP_CONFIG

libcxx/include/__random/random_device.h

 
 #if !defined(_LIBCPP_HAS_NO_RANDOM_DEVICE)
 
 class _LIBCPP_TYPE_VIS random_device
 {
 #ifdef _LIBCPP_USING_DEV_RANDOM
     int __f_;
 #elif !defined(_LIBCPP_ABI_NO_RANDOM_DEVICE_COMPATIBILITY_LAYOUT)
-#   if defined(__clang__)
-#       pragma clang diagnostic push
-#       pragma clang diagnostic ignored "-Wunused-private-field"
-#   endif
+    _LIBCPP_DIAGNOSTIC_PUSH
+    _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wunused-private-field")
 
     // Apple platforms used to use the `_LIBCPP_USING_DEV_RANDOM` code path, and now
     // use `arc4random()` as of this comment. In order to avoid breaking the ABI, we
     // retain the same layout as before.
 #   if defined(__APPLE__)
     int __padding_; // padding to fake the `__f_` field above
 #   endif
 
     // ... vendors can add workarounds here if they switch to a different representation ...
 
-#   if defined(__clang__)
-#       pragma clang diagnostic pop
-#   endif
+    _LIBCPP_DIAGNOSTIC_POP
 #endif
 
 public:
     // types
     typedef unsigned result_type;
 
     // generator characteristics
     static _LIBCPP_CONSTEXPR const result_type _Min = 0;

libcxx/include/exception

#else
     _VSTD::abort();
 #endif
 }
 
 #else // _LIBCPP_ABI_MICROSOFT
 
 class _LIBCPP_TYPE_VIS exception_ptr
 {
-#if defined(__clang__)
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wunused-private-field"
-#endif
+_LIBCPP_DIAGNOSTIC_PUSH
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wunused-private-field")
     void* __ptr1_;
     void* __ptr2_;
-#if defined(__clang__)
-#pragma clang diagnostic pop
-#endif
+_LIBCPP_DIAGNOSTIC_POP
 public:
     exception_ptr() _NOEXCEPT;
     exception_ptr(nullptr_t) _NOEXCEPT;
     exception_ptr(const exception_ptr& __other) _NOEXCEPT;
     exception_ptr& operator=(const exception_ptr& __other) _NOEXCEPT;
     exception_ptr& operator=(nullptr_t) _NOEXCEPT;
     ~exception_ptr() _NOEXCEPT;
     explicit operator bool() const _NOEXCEPT;

libcxx/include/locale

num_put<_CharT, _OutputIterator>::__do_put_integral(iter_type __s, ios_base& __iob,
     // Worst case is octal, with showbase enabled. Note that octal is always
     // printed as an unsigned value.
     using _Unsigned = typename make_unsigned<_Integral>::type;
     _LIBCPP_CONSTEXPR const unsigned __nbuf
         = (numeric_limits<_Unsigned>::digits / 3)        // 1 char per 3 bits
         + ((numeric_limits<_Unsigned>::digits % 3) != 0) // round up
         + 2; // base prefix + terminating null character
     char __nar[__nbuf];
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wformat-nonliteral"
+    _LIBCPP_DIAGNOSTIC_PUSH
+    _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wformat-nonliteral")
+    _LIBCPP_GCC_DIAGNOSTIC_IGNORED("-Wformat-nonliteral")
     int __nc = __libcpp_snprintf_l(__nar, sizeof(__nar), _LIBCPP_GET_C_LOCALE, __fmt, __v);
-#pragma clang diagnostic pop
+    _LIBCPP_DIAGNOSTIC_POP
     char* __ne = __nar + __nc;
     char* __np = this->__identify_padding(__nar, __ne, __iob);
     // Stage 2 - Widen __nar while adding thousands separators
     char_type __o[2*(__nbuf-1) - 1];
     char_type* __op;  // pad here
     char_type* __oe;  // end of output
     this->__widen_and_group_int(__nar, __np, __ne, __o, __op, __oe, __iob.getloc());
     // [__o, __oe) contains thousands_sep'd wide number

 {
     // Stage 1 - Get number in narrow char
     char __fmt[8] = {'%', 0};
     bool __specify_precision = this->__format_float(__fmt+1, __len, __iob.flags());
     const unsigned __nbuf = 30;
     char __nar[__nbuf];
     char* __nb = __nar;
     int __nc;
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wformat-nonliteral"
+    _LIBCPP_DIAGNOSTIC_PUSH
+    _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wformat-nonliteral")
+    _LIBCPP_GCC_DIAGNOSTIC_IGNORED("-Wformat-nonliteral")
     if (__specify_precision)
         __nc = __libcpp_snprintf_l(__nb, __nbuf, _LIBCPP_GET_C_LOCALE, __fmt,
                                    (int)__iob.precision(), __v);
     else
         __nc = __libcpp_snprintf_l(__nb, __nbuf, _LIBCPP_GET_C_LOCALE, __fmt, __v);
     unique_ptr<char, void(*)(void*)> __nbh(nullptr, free);
     if (__nc > static_cast<int>(__nbuf-1))
     {
         if (__specify_precision)
             __nc = __libcpp_asprintf_l(&__nb, _LIBCPP_GET_C_LOCALE, __fmt, (int)__iob.precision(), __v);
         else
             __nc = __libcpp_asprintf_l(&__nb, _LIBCPP_GET_C_LOCALE, __fmt, __v);
         if (__nc == -1)
             __throw_bad_alloc();
         __nbh.reset(__nb);
     }
-#pragma clang diagnostic pop
+    _LIBCPP_DIAGNOSTIC_POP
     char* __ne = __nb + __nc;
     char* __np = this->__identify_padding(__nb, __ne, __iob);
     // Stage 2 - Widen __nar while adding thousands separators
     char_type __o[2*(__nbuf-1) - 1];
     char_type* __ob = __o;
     unique_ptr<char_type, void(*)(void*)> __obh(0, free);
     if (__nb != __nar)
     {

libcxx/src/future.cpp

 };
 
 const char*
 __future_error_category::name() const noexcept
 {
     return "future";
 }
 
-#if defined(__clang__)
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wswitch"
-#elif defined(__GNUC__) || defined(__GNUG__)
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wswitch"
-#endif
+_LIBCPP_DIAGNOSTIC_PUSH
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wswitch")
+_LIBCPP_GCC_DIAGNOSTIC_IGNORED("-Wswitch")
 
 string
 __future_error_category::message(int ev) const
 {
     switch (static_cast<future_errc>(ev))
     {
     case future_errc(0):  // For backwards compatibility with C++11 (LWG 2056)
     case future_errc::broken_promise:
         return string("The associated promise has been destructed prior "
                       "to the associated state becoming ready.");
     case future_errc::future_already_retrieved:
         return string("The future has already been retrieved from "
                       "the promise or packaged_task.");
     case future_errc::promise_already_satisfied:
         return string("The state of the promise has already been set.");
     case future_errc::no_state:
         return string("Operation not permitted on an object without "
                       "an associated state.");
     }
     return string("unspecified future_errc value\n");
 }
 
-#if defined(__clang__)
-#pragma clang diagnostic pop
-#elif defined(__GNUC__) || defined(__GNUG__)
-#pragma GCC diagnostic pop
-#endif
+_LIBCPP_DIAGNOSTIC_POP
 
 const error_category&
 future_category() noexcept
 {
     static __future_error_category __f;
     return __f;
 }
 

libcxx/src/hash.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "__hash_table"
 #include "algorithm"
 #include "stdexcept"
 #include "type_traits"
 
-#ifdef __clang__
-#pragma clang diagnostic ignored "-Wtautological-constant-out-of-range-compare"
-#endif
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wtautological-constant-out-of-range-compare")
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 namespace {
 
 // handle all next_prime(i) for i in [1, 210), special case 0
 const unsigned small_primes[] =
 {

libcxx/src/locale.cpp

 #endif
 
 #include "include/atomic_support.h"
 #include "include/sso_allocator.h"
 #include "__undef_macros"
 
 // On Linux, wint_t and wchar_t have different signed-ness, and this causes
 // 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_CLANG_DIAGNOSTIC_IGNORED("-Wsign-conversion")
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 struct __libcpp_unique_locale {
   __libcpp_unique_locale(const char* nm) : __loc_(newlocale(LC_ALL_MASK, nm, 0)) {}
 
   ~__libcpp_unique_locale() {
     if (__loc_)
if (__loc_ == 0)
         __throw_runtime_error("time_get_byname"
                             " failed to construct for " + nm);
 }
 
 __time_get::~__time_get()
 {
     freelocale(__loc_);
 }
-#if defined(__clang__)
-#pragma clang diagnostic ignored "-Wmissing-field-initializers"
-#endif
-#if defined(__GNUG__)
-#pragma GCC   diagnostic ignored "-Wmissing-field-initializers"
-#endif
+
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wmissing-field-initializers")
 
 template <>
 string
 __time_get_storage<char>::__analyze(char fmt, const ctype<char>& ct)
 {
     tm t = {0};
     t.tm_sec = 59;
     t.tm_min = 55;
while (bb != be)
             continue;
         }
         result.push_back(*bb);
         ++bb;
     }
     return result;
 }
 
-#if defined(__clang__)
-#pragma clang diagnostic ignored "-Wmissing-braces"
-#endif
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wmissing-braces")
 
 #ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
 template <>
 wstring
 __time_get_storage<wchar_t>::__analyze(char fmt, const ctype<wchar_t>& ct)
 {
     tm t = {0};
     t.tm_sec = 59;