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

[Richard Smith]

On Wed, Jan 8, 2014 at 5:53 PM, David Wiberg <dwiberg at gmail.com> wrote:

> 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?


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


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


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.


> >> 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140108/c59d2637/attachment.html>