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

[Alp Toker]

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.

Sounds good. I'll add the extra check to make sure -Werror / -Werror=foo 
is respected in system headers and follow up here.

Alp.


-- 
http://www.nuanti.com
the browser experts