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