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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne 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')]
+            ),
----------------
Quuxplusone wrote:
> 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.
Yes, you are correct about the relationship between `-isystem` and `-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER`, and in particular the fact that we need both  D118616 and this patch in order to really enable warnings in system headers.

I also agree that it would make sense to get rid of `pragma GCC system_header` altogether -- I didn't know it was for Win32 support originally. However, I am worried that removing `pragma GCC system_header` would break some users that are using `-I include/c++/v1` and compiling with `-Werror`. Currently, they are not getting warnings from libc++ because of the pragma -- if we removed it, they would start getting those warnings (turned into errors). I suggest we take the shortest path towards re-enabling warnings, which is to land both this and D118616, and then we can go back and try to simply remove `pragma GCC system_header` altogether. I can run a build on a large code base to try to gauge the impact of it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120684