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

Wed Mar 9 09:20:00 PST 2022

Quuxplusone added inline comments.

================
Comment at: libcxx/include/__iterator/size.h:44-51
+// GCC complains about the implicit conversion from ptrdiff_t to size_t in
+// the array bound.
+_LIBCPP_DIAGNOSTIC_PUSH
+_LIBCPP_GCC_DIAGNOSTIC_IGNORED("-Wsign-conversion")
 template <class _Tp, ptrdiff_t _Sz>
 _LIBCPP_INLINE_VISIBILITY
 constexpr ptrdiff_t ssize(const _Tp (&)[_Sz]) noexcept { return _Sz; }
----------------
Quuxplusone wrote:
> I didn't notice this before. Isn't this just a "bug" on our and/or the standard's part? Shouldn't this be
> ```
> template <class _Tp, size_t _Sz>
> _LIBCPP_INLINE_VISIBILITY
> constexpr ptrdiff_t ssize(const _Tp (&)[_Sz]) noexcept { return static_cast<ptrdiff_t>(_Sz); }
> ```
> It is currently specified to take a `ptrdiff_t _Sz` parameter, but that's surprising, and if it causes compiler warnings, then we should just file an LWG issue to get it changed.
> https://eel.is/c++draft/iterator.range#lib:ssize(T_(&array)%5bN%5d)
> I'll send an email.
> 
> I think we should just preemptively change our `_Sz`'s type to suppress the warning. I don't think the change is detectable by a conforming program.
It turns out it's not that simple. :(
I'm still hoping that we could do something like
```
template <class _Tp, size_t _Sz>
#ifndef _LIBCPP_HAS_NO_CONCEPTS
  requires (_Sz < numeric_limits<ptrdiff_t>::max())
#endif
_LIBCPP_INLINE_VISIBILITY
constexpr ptrdiff_t ssize(const _Tp (&)[_Sz]) noexcept { return _Sz; }
```
but the first step is to get a regression test (working on that in D121154).

================
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:
> 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.
> 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`.

Eric replied to only the first clause of my stream-of-consciousness, which continued: "... 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."

So sure, investigate the possibility, but I bet `-Wsystem-headers` isn't going to help us in practice.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120684