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

[David Wiberg]

2014/1/8 Richard Smith <richard at metafoo.co.uk>:
> On Tue, Jan 7, 2014 at 8:33 PM, Alp Toker <alp at nuanti.com> wrote:
>>
>>
>> On 08/01/2014 04:21, Argyrios Kyrtzidis wrote:
>>>
>>> On Jan 7, 2014, at 7:56 PM, Alp Toker <alp at nuanti.com> wrote:
>>>
>>>> On 08/01/2014 01:48, Argyrios Kyrtzidis wrote:
>>>>>
>>>>> On Jan 6, 2014, at 1:47 PM, Richard Smith <richard at metafoo.co.uk
>>>>> <mailto:richard at metafoo.co.uk>> wrote:
>>>>>
>>>>>> One view on this is simply: -Wsystem-headers means "don't give system
>>>>>> headers special treatment when emitting diagnostics”.
>>>>>
>>>>> This is it exactly. “Treat all headers like normal headers"
>>>>
>>>> We don't have a flag to treat all headers as normal headers at the
>>>> moment. It'd be very simple to implement compared to -Wsystem-headers which
>>>> somewhat intricate.
>>>
>>> Could you elaborate, AFAICT '-Wsystem-headers' is treated specially, it's
>>> outside the diagnostic group machinery, and acts essentially as a flag.
>>> If you'd like to have something like '-fwarn-on-system-headers' or
>>> something instead, that's another discussion, but as far as the PR is
>>> concerned I don't see why we need to change what -Wsystem-headers is
>>> currently doing.
>>
>>
>> PR18327 reports a valid corner-case bug in the way a very small number
>> (around 8 in total) diagnostics are upgraded from warnings/extensions to
>> errors, and then were forgetting to downgrade them back to warnings as all
>> the other diagnostics are seen. So it's just an implementation detail that
>> was leaking and manifesting as errors in a context where it should have been
>> impossible.
>
>
> These diagnostics are set up as 'errors that we suppress in system headers'
> (they're suppressed because they actually happen in some system headers).
>
> If the viewpoint is that '-Wsystem-headers' means 'don't suppress
> diagnostics in system headers', then issuing errors in these cases seems
> correct to me. If the viewpoint is that '-Wsystem-headers' means 'warn on
> problems in system headers', then issuing warnings, not errors, in these
> cases seems correct. It depends on what the user of the flag intends it to
> mean.
If an error has been downgraded for system headers I don't think it
makes sense if -Wsystem-headers (which in the user's manual is
described as "Enables warnings from system headers.") causes the build
to fail. How is a system library developer supposed to check for
warnings if it isn't possible to enable warnings without breaking the
build?

>
> I think the former option makes a little more sense for system header
> developers -- if their system headers contain ill-formed code, they probably
> want to know about that with more urgency than if their system headers
> merely contain dubious code. But we still don't know what the person who
> filed the original PR was trying to do, and maybe there's a use case where
> the latter view makes more sense.
The use case for me was that I was looking at a bug report and when
reducing a test case I ended up finding a problem in lib\Headers. When
enabling -Wsystem-headers the build failed early instead of completing
and producing a warning to point me in the right direction.

>
>> Remember that this flag doesn't control the very many isInSystemHeader()
>> and isInSystemMacro() checks that happen earlier than the diagnostic
>> machinery. A -fno-system-headers flag to just disable the whole system
>> header machinery would be separately useful though, agreed.
>
>
> I agree. If we want this flag to mean 'there are no system headers', it does
> not go far enough.
>
>>
>>
>> Alp.
>>
>>
>>
>>
>>
>>>
>>>> Alp.
>>>>
>>>>>> That would seem to make perfect sense to people developing system
>>>>>> headers, and is our current behavior. What is the use case that leads to
>>>>>> enabling -Wsystem-headers but not wanting that to lead to errors? PR18327
>>>>>> doesn't make that obvious.
>>>>>
>>>>> Not sure I’m following that report, if one doesn’t like that that
>>>>> diagnostic is by default mapped to an error, maybe map it to a warning on
>>>>> the command-line or discuss whether it should not be mapped to error by
>>>>> default ?
>>>>> I don’t see a need to complicate what -Wsystem-headers does.
>>>>>
>>>> --
>>>> http://www.nuanti.com
>>>> the browser experts
>>>>
>>
>> --
>> http://www.nuanti.com
>> the browser experts
>>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>