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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__config:1440
+#  define _LIBCPP_DIAGNOSTIC_POP _Pragma("clang diagnostic pop")
+#  define _LIBCPP_CLANG_DIAGNOSTIC_IGNORED(warning) _Pragma(_LIBCPP_TOSTRING2(clang diagnostic ignored warning))
+#  define _LIBCPP_GCC_DIAGNOSTIC_IGNORED(warning)
----------------
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`.)


================
Comment at: libcxx/include/__config:1450-1451
+#  define _LIBCPP_DIAGNOSTIC_POP
+#  define _LIBCPP_GCC_DIAGNOSTIC_IGNORED(warning)
+#  define _LIBCPP_CLANG_DIAGNOSTIC_IGNORED(warning)
+#endif
----------------
Nit: Swap these two lines to keep `_LIBCPP_GCC_DIAGNOSTIC_IGNORED` and `_LIBCPP_CLANG_DIAGNOSTIC_IGNORED` in a consistent order in all three branches.


================
Comment at: libcxx/src/future.cpp:32-33
 
-#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_CLANG_DIAGNOSTIC_IGNORED("-Wswitch")
+_LIBCPP_GCC_DIAGNOSTIC_IGNORED("-Wswitch")
 
----------------
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.


================
Comment at: libcxx/src/hash.cpp:14
 
-#ifdef __clang__
-#pragma clang diagnostic ignored "-Wtautological-constant-out-of-range-compare"
-#endif
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wtautological-constant-out-of-range-compare")
 
----------------
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//).


================
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");
----------------
ldionne wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119295