[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.

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?

Alp.


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