[libcxx-commits] [PATCH] D120684: [libc++] Define _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER whenever we enable warnings in the test suite

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

Quuxplusone added subscribers: howard.hinnant, Quuxplusone.
Quuxplusone added inline comments.


================
Comment at: libcxx/utils/libcxx/test/params.py:118-123
   Parameter(name='enable_warnings', choices=[True, False], type=bool, default=True,
             help="Whether to enable warnings when compiling the test suite.",
-            actions=lambda warnings: [] if not warnings else [
-              AddOptionalWarningFlag(w) for w in _warningFlags
-            ]),
+            actions=lambda warnings: [] if not warnings else
+              [AddOptionalWarningFlag(w) for w in _warningFlags] +
+              [AddCompileFlag('-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER')]
+            ),
----------------
Do I understand correctly that this diff is intimately related to D118616?
It sounds like the status quo ante was "We use `-isystem` for our headers, but `-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER` on Clang so that in fact they //aren't// treated as system headers, thus we //do// get warnings (except, oops, `-isystem` is still in effect so we don't)".
And it sounds like the status after D118616 + D120684 will be "We use `-I` for our headers, so we get warnings, except we don't because of `pragma GCC system_header`, except (if we want warnings) we'll also pass `-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER` to nerf `pragma GCC system_header` so that we //do// get warnings."

I don't want to block either of D118616 + D120684, but I do wonder if it would be a viable followup solution to just eliminate `pragma GCC system_header` altogether. I mean, isn't the whole point of placing libc++ in a "system include directory" that the compiler //already// has heuristics for deciding what's a system header and what isn't? What was our original rationale for adding this pragma?

The pragma was already present in @howard.hinnant's first commit 3e519524c1186, and the original rationale for guarding it under `_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER` was apparently //Win32 support//, not anything at all related to warning suppression. Even the macro's name is a huge hint that we shouldn't be using it for warning suppression.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120684