<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Jan 6, 2014 at 1:02 PM, 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im"><br>
On 06/01/2014 19:27, Richard Smith wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
We need to figure out what -Wsystem-headers should do in some corner cases. In particular:<br>
<br>
* If I -Werror= a warning, and I have -Wsystem-headers, and the warning occurs in a system header, what should happen?<br>
* If I use -Werror=system-headers, and a DefaultError warning (like -Winvalid-constexpr) is issued in a system header, what should happen?<br>
* If I use -Werror=system-headers, and a (non-promoted-to-error) warning occurs in a system header, what should happen?<br>
* If I use -Werror globally, and I have -Wsystem-headers enabled, and a warning is produced in a system header, what should happen?<br>
<br>
One possible approach would be to set the apparent severity of diagnostics in system headers to max(warning severity, -Wsystem-headers severity). So -Wsystem-headers would allow warnings (but not errors) to be produced in system headers, and -Werror=system-headers would also allow errors to be produced in system headers.<br>
</blockquote>
<br></div>
Getting -W[no-]error=system-headers play ball sounds like it'd need work given what a special case it is, with limited use cases for all that flexibility. My motivation here was just to make -Wsystem-headers safe to enable at all.<br>
</blockquote><div><br></div><div>Right, I had been assuming that -Werror=system-headers already existed. But it appears that -Wsystem-headers is even more special-case than I'd thought, and doesn't act like a warning flag at all. =(</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
I suspect -Wsystem-headers -Werror should treat all header warnings as errors though. Think we have a workable solution if I update the proposed patch with that case?<br>
<br>
- // Ensure that -Wsystem-headers never introduces errors due to mapping.<br>
- Result = DiagnosticIDs::Warning;<br>
+ // Ensure that -Wsystem-headers doesn't introduce errors due to mapping.<br>
+ if (!Diag.WarningsAsErrors)<br>
+ Result = DiagnosticIDs::Warning;</blockquote><div><br></div><div>If we go this way, I think we also would want -Werror=foo -Wsystem-headers to produce diagnostic 'foo' as an error in system headers.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Another would be your current patch (never produce errors in system headers).<br>
</blockquote>
<br></div>
Just to clarify, the patch only prevents new errors being introduced by -Wsystem-headers that would not otherwise fire. It doesn't suppress any other errors that usually fire in system headers.</blockquote><div><br></div>
<div>Sorry, I was unclear here; I meant "never emit Warning diagnostics as errors in system headers".</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Another would be to treat all warnings in system headers as if they're controlled by the -Wsystem-headers flag (so -Werror=system-headers would promote them all to errors, as would -Werror -Wsystem-headers).<br>
</blockquote>
<br></div>
Would that need early checks on the warning location to see if it's in system headers? The check is described as expensive so I've left it until just before emission.</blockquote><div><br></div><div>No, I meant that you'd still perform the check in the same place; only warnings that would be displayed outside a system header would get the additional check. But I don't think this option is the right approach; it doesn't seem very useful from a user's point of view.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class=""><font color="#888888">
Alp.<br>
<br>
<br>
<br>
</font></span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">
<br>
Maybe there's another, better, option.<br></div></blockquote></blockquote><div><br></div><div>One other option would be to make no change. Ultimately, what I was trying to drive towards with my earlier questions was, what is the semantic model for -Wsystem-headers? What does a user expect it to mean?</div>
<div><br></div><div>One view on this is simply: -Wsystem-headers means "don't give system headers special treatment when emitting diagnostics". 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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im">
Thoughts?<br>
<br></div><div class="im">
On Sun Dec 29 2013 at 7:01:45 AM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a> <mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>>> wrote:<br>
<br>
The -Wsystem-headers option was enabling warnings that got upgraded to<br>
errors<br>
through mappings like DefaultError. In a normal build these errors<br>
are fully<br>
suppressed.<br>
<br>
This patch makes -Wsystem-headers consistent with ordinary<br>
behaviour by<br>
restoring mapped errors in system headers to warnings, ensuring<br>
that the<br>
option<br>
can never cause build failures.<br>
<br>
The test case extends existing checks added in r169689 / PR14550.<br>
<br>
Alp.<br>
<br>
--<br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
</div></blockquote><div class=""><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>