[PATCH] PR18327: -Wsystem-headers introduces build errors

[Richard Smith]

On Wed Jan 29 2014 at 1:45:22 PM, Alp Toker <alp at nuanti.com> wrote:

>
> On 09/01/2014 23:26, Richard Smith wrote:
> > On Thu, Jan 9, 2014 at 3:18 PM, Marshall Clow <mclow.lists at gmail.com
> > <mailto:mclow.lists at gmail.com>> wrote:
> >
> >
> >     On Jan 9, 2014, at 2:51 PM, Richard Smith <richard at metafoo.co.uk
> >     <mailto:richard at metafoo.co.uk>> wrote:
> >
> >>     On Thu, Jan 9, 2014 at 2:39 PM, Marshall Clow
> >>     <mclow.lists at gmail.com <mailto:mclow.lists at gmail.com>> wrote:
> >>
> >>         On Jan 9, 2014, at 2:20 PM, Richard Smith
> >>         <richard at metafoo.co.uk <mailto:richard at metafoo.co.uk>> wrote:
> >>
> >>>         On Thu, Jan 9, 2014 at 7:25 AM, Alp Toker <alp at nuanti.com
> >>>         <mailto:alp at nuanti.com>> wrote:
> >>>
> >>>             It's not fair on the user or the system header
> >>>             maintainers to bring up unexpected errors with this flag.
> >>>
> >>>
> >>>         I don't think this is clear. If the code in the system
> >>>         header is ill-formed, I would expect the system header
> >>>         maintainers to be *grateful* that we flag it as an error and
> >>>         not just as a warning. But I'll wait to hear what they have
> >>>         to say about that.
> >>
> >>         A few points, in no particular order:
> >>
> >>         * If we have ill-formed code in system headers, I would
> >>         expect it to fail to compile whether the user specifies
> >>         -Wsystem-headers or not.
> >>
> >>
> >>     And yet in some cases we don't, because we deliberately support
> >>     some flavors of broken system headers.
> >>     Given that, would you want -Wsystem-headers to make such code
> >>     produce an error or only a warning?
> >
> >     Eww.
> >
> >     First - is any of that code in libc++?
> >     Second, there's two audiences that I think are affected here, and
> >     they have different needs:
> >     * People trying to use the system headers. These people are, in
> >     general, unable/unwilling to change them. Giving them
> >     warnings/errors is just noise.
> >     * People who are working on the system headers. They need to see
> >     the warnings.
> >
> >
> >>
> >>         * If a user has a clean build, and then rebuilds with
> >>         -Wsystem-headers, I would expect to get warnings - not
> >>         errors. [ Isn't that what PR18327 is all about? ]
> >>
> >>
> >>     Do you expect this because the flag starts with -W?
> >
> >     Yes.
> >
> >>     (Maybe that's the problem here -- it's *not* a warning flag in
> >>     the sense that the other -W flags are. But that's not
> >>     unprecedented -- nor is -Werror.)
> >
> >
> >>         * There are some "interesting" language features which are
> >>         only enabled for system headers, and cause warnings if used
> >>         in user code.
> >>         [ User-defined suffixes that do not start with an underscore,
> >>         for example. ]
> >>
> >>
> >>     This is a really great example, thanks.
> >
> >     Note that putting something like this in your source file gives
> >     you a warning;
> >
> >         std::complex<long double> operator"" ilx(long double __im)
> >         {
> >             return { 0.0l, __im };
> >         }
> >
> >     $ clang11 junk.cpp
> >     junk.cpp:5:31: warning: user-defined literal suffixes not starting
> >     with '_' are
> >           reserved; no literal will invoke this operator
> >     [-Wuser-defined-literals]
> >         std::complex<long double> operator"" ilx(long double __im)
> >                                   ^
> >     1 warning generated.
> >
> >
> >>     So it seems there are at least three different classes of errors
> >>     that we might think about producing in system headers:
> >>
> >>      1) warnings that the user has turned into errors with -Werror or
> >>     -Werror=foo
> >>      2) errors for ill-formed code that we suppress by default in
> >>     system headers as a workaround for a system header bug
> >>      3) errors for code that is ill-formed outside system headers but
> >>     valid within system headers (using reserved names, adding names
> >>     to namespace std, that sort of thing)
> >>
> >>     With -Wsystem-headers enabled, I think (1) should be an error,
> >>     (3) should remain suppressed (not even a warning), and (2) should
> >>     be either a warning or an error (and your first two bullets don't
> >>     give me a clear idea of which way these cases should go).
> >
> >     Agreed for #1 and #3.
> >     And for #2, I'd like to see a warning;  "This is code we wouldn't
> >     allow outside a system header, but we do so here"
> >
> >     I don't see the point of making #2 an error.
> >     For most people (who can't fix it), it's just a hassle.
> >     For system developers, they can read warning output just as well
> >     as error output.
> >
> >
> > OK, so I think this is Alp's original patch plus a change to treat #1
> > as an error. As you observe, -Wuser-defined-literals is currently not
> > an error outside of a system header, so perhaps we have no diagnostics
> > in case #3 at the moment, but it's something to keep in mind for the
> > future.
>
> Status update: I did implement those semantics and it doesn't make as
> much sense in practice as we'd hoped.
>
> The problem is that many conventional flags such as a -Werror build or
> specific -Werror upgrades will now consistently cause system headers to
> break the build, so it's no better than where we started, and arguably
> worse.
>
> As a result I'm back with proposing the original patch for PR18327 as-is
> with more confidence. It's the only mode I've found through several
> iterations that adds informational/educational value in a way that users
> and system header developers can safely consume.
>
> (There is good news though, I've got some good cleanups and fixes out of
> the investigation that I'll post separately, especially in the area of
> "explaining" the source of diagnostics which is currently a hack in ToT.)
>
> Richard, does that work for you?


I don't think I'm following. Without your patch, -Werror -Wsystem-headers
was an error; why is it bad for it to stay that way?

-Werror is the way users say "I want a broken build if you find a warning".
This change seems to break that.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140129/0f62e381/attachment.html>