Move compiler warning flags into a header
Categories
(Firefox Build System :: General, task)
Tracking
(Not tracked)
People
(Reporter: glandium, Assigned: glandium)
Details
With a subtle use of #if
and #pragma
, we can enable warning flags from an include file that could be included from both mozilla-config.h
and js-confdefs.h
, avoiding a bunch of tests during configure.
That would also shorten compiler command lines drastically.
Assignee | ||
Comment 1•4 years ago
|
||
So far, this is what the header would look like:
// Clang support `__has_warning`, but GCC doesn't.
#ifndef __has_warning
// For GCC, pretend that `__has_warning` exists and is true for all flags, and ignore pragma warnings.
#define __has_warning(x) 1
#pragma GCC diagnostic ignored "-Wpragmas"
#endif
#if __has_warning("-Wfoo")
#ifdef WARNINGS_AS_ERRORS
#pragma GCC diagnostic error "-Wfoo"
#else
#pragma GCC diagnostic warning "-Wfoo"
#endif
#endif
// Restore pragma warnings.
#pragma GCC diagnostic warning "-Wpragmas"
Sadly it's not possible to do something like
#define LEVEL warning
#pragma GCC diagnostic LEVEL "-Wfoo"
so we do have to go with the ifdefs.
Other things to consider:
AllowCompilerWarnings
, which currently removes the warning-as-errors flagsDisableCompilerWarnings
, which disables all warnings- Ad-hoc
-Wno-error=foo
or-Wno-foo
in some directories
So if we go forward with this idea, it will mean a lot of #ifdef
'ing around. Obviously, we can have this header generated (that would be the only sustainable way to deal with this).
Nathan, do you have thoughts?
Unsolicited, but here's my take. I'm really excited to reduce configure time, configure spam, command lines, and CI build spam.
Having to do gymnastics in the header is a bit unpleasant, but isn't the worst thing in the world. And perhaps we could reduce that even further by tailoring the header to the compiler and -Werror setting at configure time? If the header could be just a sheet of consecutive pragmas, that would be quite easy to read and understand.
I have one concern, about debugging build failures. It's already irritating enough to developers when they get surprised by a -Werror in automation that didn't show up locally. I'm a little worried that this header might be an extra source of surprise / non-obviousness / tribal knowledge that you "just have to know". [Edit to add -- I mean this in the sense of "I ran the same command line as automation, why doesn't it fail on my machine?"] I don't know how we could mitigate that, maybe a dev-platform announcement and lots of docs, but I'm not sure.
Comment 3•4 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #1)
Sadly it's not possible to do something like
#define LEVEL warning #pragma GCC diagnostic LEVEL "-Wfoo"
so we do have to go with the ifdefs.
Can we make this pattern work with _Pragma
instead, since I think that's there specifically to make preprocessing hacks work? http://eel.is/c++draft/cpp.pragma.op
Other things to consider:
AllowCompilerWarnings
, which currently removes the warning-as-errors flagsDisableCompilerWarnings
, which disables all warnings- Ad-hoc
-Wno-error=foo
or-Wno-foo
in some directories
I think the first two are doable, perhaps with extra defines passed in that our magic warnings header understands.
The third one I am not sure about, because I'm guessing the header overrides any directives from moz.build
/the command-line, which is not desirable. I guess if we were already in #ifdef
territory, we could make the frontend emit magic directives that would control the presence of the relevant warnings in the header if moz.build
enabled or disabled certain warnings?
(In reply to :dmajor from comment #2)
I have one concern, about debugging build failures. It's already irritating enough to developers when they get surprised by a -Werror in automation that didn't show up locally. I'm a little worried that this header might be an extra source of surprise / non-obviousness / tribal knowledge that you "just have to know". [Edit to add -- I mean this in the sense of "I ran the same command line as automation, why doesn't it fail on my machine?"] I don't know how we could mitigate that, maybe a dev-platform announcement and lots of docs, but I'm not sure.
I can understand the spooky action at a distance concerns, but I think that by and large, developers have acclimated themselves to running with the same compiler that automation uses, so I would expect relatively few issues to show up along those lines. And if they don't, I think bootstrap has made it much easier to at least run with the exact same compiler version as automation, so I would expect that most of those relatively few concerns can be resolved without much difficulty.
Assignee | ||
Comment 4•4 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #3)
(In reply to Mike Hommey [:glandium] from comment #1)
Sadly it's not possible to do something like
#define LEVEL warning #pragma GCC diagnostic LEVEL "-Wfoo"
so we do have to go with the ifdefs.
Can we make this pattern work with
_Pragma
instead, since I think that's there specifically to make preprocessing hacks work? http://eel.is/c++draft/cpp.pragma.op
I was going to say "but that's C++", but it looks like it works in C too.
Comment 5•4 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #4)
(In reply to Nathan Froyd [:froydnj] from comment #3)
Can we make this pattern work with
_Pragma
instead, since I think that's there specifically to make preprocessing hacks work? http://eel.is/c++draft/cpp.pragma.opI was going to say "but that's C++", but it looks like it works in C too.
And the C committee has been kind enough to define it in C99, so it's actually relatively recent in C terms.
Description
•