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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: libcxx/include/__config:145-152
+#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)
----------------
I don't think this is intended to be part of this diff. This looks like a rebase issue to me.


================
Comment at: libcxx/include/__config:1433
+#  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)
----------------
This should use `_LIBCPP_TOSTRING`, not `_LIBCPP_TOSTRING2` (ditto below).


================
Comment at: libcxx/src/locale.cpp:5202
+
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wmissing-field-initializers")
 
----------------
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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119295