On Thu, Jan 9, 2014 at 7:25 AM, Alp Toker <[email protected]> wrote: > > On 09/01/2014 03:29, Richard Smith wrote: > >> On Wed, Jan 8, 2014 at 5:53 PM, David Wiberg <[email protected] <mailto: >> [email protected]>> wrote: >> >> 2014/1/8 Richard Smith <[email protected] >> <mailto:[email protected]>>: >> >> > On Tue, Jan 7, 2014 at 8:33 PM, Alp Toker <[email protected] >> <mailto:[email protected]>> wrote: >> >> >> >> >> >> On 08/01/2014 04:21, Argyrios Kyrtzidis wrote: >> >>> >> >>> On Jan 7, 2014, at 7:56 PM, Alp Toker <[email protected] >> <mailto:[email protected]>> wrote: >> >>> >> >>>> On 08/01/2014 01:48, Argyrios Kyrtzidis wrote: >> >>>>> >> >>>>> On Jan 6, 2014, at 1:47 PM, Richard Smith >> <[email protected] <mailto:[email protected]> >> >>>>> <mailto:[email protected] >> >> <mailto:[email protected]>>> 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. >> > > Hi Richard, > > I've taken a step back to look at this in context. > > David Wiberg's end-user use case appears entirely reasonable to me,
I agree. He can achieve his use case with -Wsystem-headers -Wno-error=invalid-constexpr. > and there's a strong expectation that -Wsystem-headers shouldn't introduce > errors in valid system header code. 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. > 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. > 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. > > 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. > > -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. > > Anything else will draw unwanted problem reports against the standard > library implementation. > > For these reasons I'm happy with my original patch as a conservatively > correct step in the right direction. > > 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. > > 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. > > Alp. > > > > >> 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 >> > [email protected] <mailto:[email protected]> >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> > >> >> > -- > http://www.nuanti.com > the browser experts > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
