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