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

[Richard Smith]

On Thu, Jan 9, 2014 at 3:18 PM, Marshall Clow <mclow.lists at gmail.com> wrote:

>
> On Jan 9, 2014, at 2:51 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>
> On Thu, Jan 9, 2014 at 2:39 PM, Marshall Clow <mclow.lists at gmail.com>wrote:
>
>> On Jan 9, 2014, at 2:20 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>>
>> On Thu, Jan 9, 2014 at 7:25 AM, Alp Toker <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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140109/8719affd/attachment.html>