<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Jan 9, 2014 at 7:25 AM, Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On 09/01/2014 03:29, Richard Smith wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
On Wed, Jan 8, 2014 at 5:53 PM, David Wiberg <<a href="mailto:dwiberg@gmail.com" target="_blank">dwiberg@gmail.com</a> <mailto:<a href="mailto:dwiberg@gmail.com" target="_blank">dwiberg@gmail.com</a>>> wrote:<br>
<br>
2014/1/8 Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a><br></div>
<mailto:<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>><u></u>>:<div class="im"><br>
> On Tue, Jan 7, 2014 at 8:33 PM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a><br></div><div class="im">
<mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>>> wrote:<br>
>><br>
>><br>
>> On 08/01/2014 04:21, Argyrios Kyrtzidis wrote:<br>
>>><br>
>>> On Jan 7, 2014, at 7:56 PM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a><br></div><div class="im">
<mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>>> wrote:<br>
>>><br>
>>>> On 08/01/2014 01:48, Argyrios Kyrtzidis wrote:<br>
>>>>><br>
>>>>> On Jan 6, 2014, at 1:47 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>><br></div>
>>>>> <mailto:<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a><div><div class="h5"><br>
<mailto:<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>><u></u>>> wrote:<br>
>>>>><br>
>>>>>> One view on this is simply: -Wsystem-headers means "don't<br>
give system<br>
>>>>>> headers special treatment when emitting diagnostics”.<br>
>>>>><br>
>>>>> This is it exactly. “Treat all headers like normal headers"<br>
>>>><br>
>>>> We don't have a flag to treat all headers as normal headers<br>
at the<br>
>>>> moment. It'd be very simple to implement compared to<br>
-Wsystem-headers which<br>
>>>> somewhat intricate.<br>
>>><br>
>>> Could you elaborate, AFAICT '-Wsystem-headers' is treated<br>
specially, it's<br>
>>> outside the diagnostic group machinery, and acts essentially<br>
as a flag.<br>
>>> If you'd like to have something like '-fwarn-on-system-headers' or<br>
>>> something instead, that's another discussion, but as far as<br>
the PR is<br>
>>> concerned I don't see why we need to change what<br>
-Wsystem-headers is<br>
>>> currently doing.<br>
>><br>
>><br>
>> PR18327 reports a valid corner-case bug in the way a very small<br>
number<br>
>> (around 8 in total) diagnostics are upgraded from<br>
warnings/extensions to<br>
>> errors, and then were forgetting to downgrade them back to<br>
warnings as all<br>
>> the other diagnostics are seen. So it's just an implementation<br>
detail that<br>
>> was leaking and manifesting as errors in a context where it<br>
should have been<br>
>> impossible.<br>
><br>
><br>
> These diagnostics are set up as 'errors that we suppress in<br>
system headers'<br>
> (they're suppressed because they actually happen in some system<br>
headers).<br>
><br>
> If the viewpoint is that '-Wsystem-headers' means 'don't suppress<br>
> diagnostics in system headers', then issuing errors in these<br>
cases seems<br>
> correct to me. If the viewpoint is that '-Wsystem-headers' means<br>
'warn on<br>
> problems in system headers', then issuing warnings, not errors,<br>
in these<br>
> cases seems correct. It depends on what the user of the flag<br>
intends it to<br>
> mean.<br>
If an error has been downgraded for system headers I don't think it<br>
makes sense if -Wsystem-headers (which in the user's manual is<br>
described as "Enables warnings from system headers.") causes the build<br>
to fail. How is a system library developer supposed to check for<br>
warnings if it isn't possible to enable warnings without breaking the<br>
build?<br>
<br>
<br>
They're supposed to fix their code so it's valid C++ =) (and they can use the warning flag name we list, to turn the error off).<br>
<br>
> I think the former option makes a little more sense for system<br>
header<br>
> developers -- if their system headers contain ill-formed code,<br>
they probably<br>
> want to know about that with more urgency than if their system<br>
headers<br>
> merely contain dubious code. But we still don't know what the<br>
person who<br>
> filed the original PR was trying to do, and maybe there's a use<br>
case where<br>
> the latter view makes more sense.<br>
The use case for me was that I was looking at a bug report and when<br>
reducing a test case I ended up finding a problem in lib\Headers. When<br>
enabling -Wsystem-headers the build failed early instead of completing<br>
and producing a warning to point me in the right direction.<br>
<br>
<br>
OK, and because the error caused your build to fail, it was harder for you to perform the reduction? That's an interesting situation, but it seems pretty easy to work around once you know what's happening. I'm still on the fence here.<br>
</div></div></blockquote>
<br>
Hi Richard,<br>
<br>
I've taken a step back to look at this in context.<br>
<br>
David Wiberg's end-user use case appears entirely reasonable to me,</blockquote><div><br></div><div>I agree. He can achieve his use case with -Wsystem-headers -Wno-error=invalid-constexpr.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
and there's a strong expectation that -Wsystem-headers shouldn't introduce errors in valid system header code.</blockquote><div><br></div><div>I don't think this has been demonstrated. We have only one report of a user being surprised here, and at least one Clang developer believes the current behavior is appropriate.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> It's not fair on the user or the system header maintainers to bring up unexpected errors with this flag.<br>
</blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">In this case, the problem was that the flag was inadvertently disabling a system header workaround that you introduced, and that's not really the purpose of the flag to disable compatibility features. It does appear just to be a bug that my patch resolves.<br>
<br>
We absolutely should have a -fno-system-headers that simply disables special handling of system headers and it'll be an invaluable tool for standard library developers. But this is not the right flag for that.<br>
<br>
-Wsystem-headers should have minimal impact beyond doing "what it says on the tin" so that interested developers can explore the system header workarounds and warnings safely without risking a broken build.<br>
<br>
Anything else will draw unwanted problem reports against the standard library implementation.<br>
<br>
For these reasons I'm happy with my original patch as a conservatively correct step in the right direction.<br>
<br>
Some background: I've realized after discussions with the libc++ developers that a small change like this can be a headache for system header developers. This has made me very conscious that we take care to preserve any workarounds and not silently upgrade them to hard errors in user code that ordinary developers will encounter.<br>
<br>
So I see it as part of the unspoken contract we have with the libc++ team to fix this in the conservative way I outlined in PR18327, such that the flag doesn't introduce errors for code that's otherwise accepted.<br>
<br>
Alp.<br>
<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
>> Remember that this flag doesn't control the very many<br>
isInSystemHeader()<br>
>> and isInSystemMacro() checks that happen earlier than the<br>
diagnostic<br>
>> machinery. A -fno-system-headers flag to just disable the whole<br>
system<br>
>> header machinery would be separately useful though, agreed.<br>
><br>
><br>
> I agree. If we want this flag to mean 'there are no system<br>
headers', it does<br>
> not go far enough.<br>
><br>
>><br>
>><br>
>> Alp.<br>
>><br>
>><br>
>><br>
>><br>
>><br>
>>><br>
>>>> Alp.<br>
>>>><br>
>>>>>> That would seem to make perfect sense to people developing<br>
system<br>
>>>>>> headers, and is our current behavior. What is the use case<br>
that leads to<br>
>>>>>> enabling -Wsystem-headers but not wanting that to lead to<br>
errors? PR18327<br>
>>>>>> doesn't make that obvious.<br>
>>>>><br>
>>>>> Not sure I’m following that report, if one doesn’t like that<br>
that<br>
>>>>> diagnostic is by default mapped to an error, maybe map it to<br>
a warning on<br>
>>>>> the command-line or discuss whether it should not be mapped<br>
to error by<br>
>>>>> default ?<br>
>>>>> I don’t see a need to complicate what -Wsystem-headers does.<br>
>>>>><br>
>>>> --<br>
>>>> <a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
>>>> the browser experts<br>
>>>><br>
>><br>
>> --<br>
>> <a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
>> the browser experts<br>
>><br>
><br>
><br>
> ______________________________<u></u>_________________<br>
> cfe-commits mailing list<br></div></div>
> <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a> <mailto:<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.<u></u>edu</a>><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/cfe-commits</a><br>
><br>
<br>
</blockquote><div class="HOEnZb"><div class="h5">
<br>
-- <br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
</div></div></blockquote></div><br></div></div>