[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 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')]
+            ),
----------------
ldionne wrote:
> 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.
> 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`.

Good point; I agree with that concern.

> I suggest we take the shortest path towards re-enabling warnings

Seems like warnings have been disabled since, like, forever, haven't they? so I'm temperamentally inclined to say "let's not move until we know exactly where we're going." But I don't actually wish to block your plan; go for it AFAIC.

One more thought: Are we sure there's no existing Clang/GCC compiler option like `-Weven-in-system-headers`? That's really what we... oh, bah, no, that's //not// what we want. We want to see errors from //libc++// system headers //only//, but continue suppressing any errors from e.g. `/usr/include/stdio.h`. Yuck. In that case, your plan might get us to the best possible state, except that `_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER` will be somewhat misnamed; at this point it would be more like a `_LIBCPP_DONT_SUPPRESS_WARNINGS`.

TLDR, go for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120684