<div>On Wed Jan 29 2014 at 4:03:06 PM, Alp Toker <<a href="mailto:alp@nuanti.com">alp@nuanti.com</a>> wrote:</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On 29/01/2014 23:25, Richard Smith wrote:<br>
> On Wed Jan 29 2014 at 1:45:22 PM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a><br>
> <mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>>> wrote:<br>
><br>
><br>
> On 09/01/2014 23:26, Richard Smith wrote:<br>
> > On Thu, Jan 9, 2014 at 3:18 PM, Marshall Clow<br>
> <<a href="mailto:mclow.lists@gmail.com" target="_blank">mclow.lists@gmail.com</a> <mailto:<a href="mailto:mclow.lists@gmail.com" target="_blank">mclow.lists@gmail.com</a>><br>
> > <mailto:<a href="mailto:mclow.lists@gmail.com" target="_blank">mclow.lists@gmail.com</a> <mailto:<a href="mailto:mclow.lists@gmail.com" target="_blank">mclow.lists@gmail.com</a>><u></u>>><br>
> wrote:<br>
> ><br>
> ><br>
> > On Jan 9, 2014, at 2:51 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a><br>
> <mailto:<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>><br>
> > <mailto:<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>><u></u>>><br>
> wrote:<br>
> ><br>
> >> On Thu, Jan 9, 2014 at 2:39 PM, Marshall Clow<br>
> >> <<a href="mailto:mclow.lists@gmail.com" target="_blank">mclow.lists@gmail.com</a> <mailto:<a href="mailto:mclow.lists@gmail.com" target="_blank">mclow.lists@gmail.com</a>><br>
> <mailto:<a href="mailto:mclow.lists@gmail.com" target="_blank">mclow.lists@gmail.com</a> <mailto:<a href="mailto:mclow.lists@gmail.com" target="_blank">mclow.lists@gmail.com</a>><u></u>>> wrote:<br>
> >><br>
> >> On Jan 9, 2014, at 2:20 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>
> <mailto:<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>><u></u>>> wrote:<br>
> >><br>
> >>> On Thu, Jan 9, 2014 at 7:25 AM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a><br>
> <mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>><br>
> >>> <mailto:<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>
> >>> It's not fair on the user or the system header<br>
> >>> maintainers to bring up unexpected errors with this flag.<br>
> >>><br>
> >>><br>
> >>> I don't think this is clear. If the code in the system<br>
> >>> header is ill-formed, I would expect the system header<br>
> >>> maintainers to be *grateful* that we flag it as an error and<br>
> >>> not just as a warning. But I'll wait to hear what they have<br>
> >>> to say about that.<br>
> >><br>
> >> A few points, in no particular order:<br>
> >><br>
> >> * If we have ill-formed code in system headers, I would<br>
> >> expect it to fail to compile whether the user specifies<br>
> >> -Wsystem-headers or not.<br>
> >><br>
> >><br>
> >> And yet in some cases we don't, because we deliberately support<br>
> >> some flavors of broken system headers.<br>
> >> Given that, would you want -Wsystem-headers to make such code<br>
> >> produce an error or only a warning?<br>
> ><br>
> > Eww.<br>
> ><br>
> > First - is any of that code in libc++?<br>
> > Second, there’s two audiences that I think are affected here, and<br>
> > they have different needs:<br>
> > * People trying to use the system headers. These people are, in<br>
> > general, unable/unwilling to change them. Giving them<br>
> > warnings/errors is just noise.<br>
> > * People who are working on the system headers. They need to see<br>
> > the warnings.<br>
> ><br>
> ><br>
> >><br>
> >> * If a user has a clean build, and then rebuilds with<br>
> >> -Wsystem-headers, I would expect to get warnings - not<br>
> >> errors. [ Isn’t that what PR18327 is all about? ]<br>
> >><br>
> >><br>
> >> Do you expect this because the flag starts with -W?<br>
> ><br>
> > Yes.<br>
> ><br>
> >> (Maybe that's the problem here -- it's *not* a warning flag in<br>
> >> the sense that the other -W flags are. But that's not<br>
> >> unprecedented -- nor is -Werror.)<br>
> ><br>
> ><br>
> >> * There are some “interesting” language features which are<br>
> >> only enabled for system headers, and cause warnings if used<br>
> >> in user code.<br>
> >> [ User-defined suffixes that do not start with an underscore,<br>
> >> for example. ]<br>
> >><br>
> >><br>
> >> This is a really great example, thanks.<br>
> ><br>
> > Note that putting something like this in your source file gives<br>
> > you a warning;<br>
> ><br>
> > std::complex<long double> operator"" ilx(long double __im)<br>
> > {<br>
> > return { 0.0l, __im };<br>
> > }<br>
> ><br>
> > $ clang11 junk.cpp<br>
> > junk.cpp:5:31: warning: user-defined literal suffixes not starting<br>
> > with '_' are<br>
> > reserved; no literal will invoke this operator<br>
> > [-Wuser-defined-literals]<br>
> > std::complex<long double> operator"" ilx(long double __im)<br>
> > ^<br>
> > 1 warning generated.<br>
> ><br>
> ><br>
> >> So it seems there are at least three different classes of errors<br>
> >> that we might think about producing in system headers:<br>
> >><br>
> >> 1) warnings that the user has turned into errors with -Werror or<br>
> >> -Werror=foo<br>
> >> 2) errors for ill-formed code that we suppress by default in<br>
> >> system headers as a workaround for a system header bug<br>
> >> 3) errors for code that is ill-formed outside system headers but<br>
> >> valid within system headers (using reserved names, adding names<br>
> >> to namespace std, that sort of thing)<br>
> >><br>
> >> With -Wsystem-headers enabled, I think (1) should be an error,<br>
> >> (3) should remain suppressed (not even a warning), and (2) should<br>
> >> be either a warning or an error (and your first two bullets don't<br>
> >> give me a clear idea of which way these cases should go).<br>
> ><br>
> > Agreed for #1 and #3.<br>
> > And for #2, I’d like to see a warning; “This is code we wouldn’t<br>
> > allow outside a system header, but we do so here”<br>
> ><br>
> > I don’t see the point of making #2 an error.<br>
> > For most people (who can’t fix it), it’s just a hassle.<br>
> > For system developers, they can read warning output just as well<br>
> > as error output.<br>
> ><br>
> ><br>
> > OK, so I think this is Alp's original patch plus a change to<br>
> treat #1<br>
> > as an error. As you observe, -Wuser-defined-literals is<br>
> currently not<br>
> > an error outside of a system header, so perhaps we have no<br>
> diagnostics<br>
> > in case #3 at the moment, but it's something to keep in mind for the<br>
> > future.<br>
><br>
> Status update: I did implement those semantics and it doesn't make as<br>
> much sense in practice as we'd hoped.<br>
><br>
> The problem is that many conventional flags such as a -Werror build or<br>
> specific -Werror upgrades will now consistently cause system<br>
> headers to<br>
> break the build, so it's no better than where we started, and arguably<br>
> worse.<br>
><br>
> As a result I'm back with proposing the original patch for PR18327<br>
> as-is<br>
> with more confidence. It's the only mode I've found through several<br>
> iterations that adds informational/educational value in a way that<br>
> users<br>
> and system header developers can safely consume.<br>
><br>
> (There is good news though, I've got some good cleanups and fixes<br>
> out of<br>
> the investigation that I'll post separately, especially in the area of<br>
> "explaining" the source of diagnostics which is currently a hack<br>
> in ToT.)<br>
><br>
> Richard, does that work for you?<br>
><br>
><br>
> I don't think I'm following. Without your patch, -Werror<br>
> -Wsystem-headers was an error; why is it bad for it to stay that way?<br>
><br>
> -Werror is the way users say "I want a broken build if you find a<br>
> warning". This change seems to break that.<br>
<br>
These are the complications:<br>
<br>
1) We skip a whole lot of warnings with isInSystemHeader() early checks<br>
anyway. A more valid way to debug system headers for correctness would<br>
be to include them without -isystem, or provide a -fno-system-headers<br>
flag instead of making this flag try to simulate that behaviour. I don't<br>
like the idea of representing this flag as that or making it feel in any<br>
way like an ordinary warning flag because what it does is unusual.<br></blockquote><div><br></div><div>I find this argument persuasive. A (-f) flag to say 'there are no system headers' seems like a more reliable way of getting that effect than the hit-and-miss result of -Wsystem-headers, if anyone actually wants that behavior.</div>
<div><br></div><div>(Just telling people not to use -isystem is impractical; switching includes between -I and isystem has various subtle consequences due to changes in include path order, and they'd have to manually replicate the -isystem paths we implicitly inject.)</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
2) System headers use, either intentionally or for legacy reasons, code<br>
that's always out of sync with the user's language mode and settings.<br>
libc++ uses C++11 extensions even in earlier language standards, which<br>
is a great idea. MSVC headers use MS extensions, which is also par for<br>
the course. -Werror would just make every build break without providing<br>
much insight.<br></blockquote><div><br></div><div>That's a good point.</div><div><br></div><div>That said, lots of other code gets treated as system headers too, by various projects (usually third-party dependencies, sometimes just crufty old first-party code). If someone discovers a new warning that's finding bugs in their code, and they also want to look for bugs in their dependencies (including system headers and third-party code), that seems like something we should be able to support. Being able to run with -Wsystem-headers -Werror=my-new-favourite-warning is practically useful today (apart from issues where -Wsystem-headers enables errors of other kinds, which I agree should not be errors). But the new behavior isn't *that* much worse (you can't find the diagnostics by looking for build breaks, but you can still grep through the compiler output or whatever).</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
$ cat test.cpp<br>
#include <type_traits><br>
<br>
$ clang -std=c++98 -Werror test.cpp<br>
<br>
$ clang -std=c++98 -Werror -Wsystem-headers test.cpp<br>
In file included from werror.cpp:1:<br>
In file included from<br>
/Users/alp/Projects/llvm-work/<u></u>build-upstream-ninja/bin/../<u></u>include/c++/v1/type_traits:<u></u>202:<br>
include/c++/v1/__config:346:3: error:<br>
inline namespaces are a C++11 feature [-Werror,-Wc++11-extensions]<br>
inline namespace _LIBCPP_NAMESPACE {<br>
^<br>
<br>
For these two reasons, I think any error raised by -Werror<br>
-Wsystem-headers would be unactionable and I don't see why anyone would<br>
want the mode. It's a non-goal for system headers to build cleanly<br>
without warnings in any configuration we support today. Indeed we<br>
intentionally try to highlight quirks in system headers with great<br>
verbosity.<br>
<br>
My intended use case with this flag is to have a way to document<br>
"interesting facts" about system headers, and David Wiberg seems to be a<br>
consumer for that use case, so I'm interested in toning it down to a<br>
"fun" feature that people can use to understand what's going on.<br>
<br>
So the semantics of -Wsystem-headers would be "show diagnostics in<br>
system headers that would otherwise be suppressed as warnings."<br>
<br>
It is quirky, but I think less so than pretending that these are<br>
equivalent to ordinary warning diagnostics in user code. Sometimes a<br>
quirky problem demands a quirky solution :-)<br></blockquote><div><br></div><div>OK, I think you've sold me on this.</div>