[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 marked 3 inline comments as done.
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')]
+            ),
----------------
EricWF wrote:
> Quuxplusone wrote:
> > 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.
> No, warnings have not been disabled since forever. I was quite careful to not that that happen prior to my departure when COVID started.
> 
> >  Are we sure there's no existing Clang/GCC compiler option like -Weven-in-system-headers
> 
> You mean `-Wsystem-headers`? 
Oooh, I wasn't aware of that flag. This is really neat, and in fact I think it does remove the need for `_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER`. I suggest:

1. I finish and ship this patch so that we have warnings enabled on all compilers, in all configurations.
2. I go back and investigate the removal of `_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER`, changing `-I` to `-isystem` in our test suite and adding `-Wsystem-headers`. That would be equivalent to what we have after (1) but simpler.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120684