Closed
Bug 903663
Opened 11 years ago
Closed 11 years ago
Use MOZ_C_SUPPORTS_WARNING and MOZ_CXX_SUPPORTS_WARNING to exempt warnings from FAIL_ON_WARNINGS
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla26
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
We currently exempt gcc/clang warnings from FAIL_ON_WARNINGS by directly passing "-Wno-error=whatever". (See patches from bug 716541, bug 833405 for example)
This is bad in cases where the warning only exists in clang, or only exists in GCC, or only exists in certain versions. If it's unsupported, then the compiler will crap out if we pass "-Wno-error=something-unsupported", as I discovered in bug 903513 comment 8.
GOOD NEWS: as njn hinted at, we can use MOZ_C_SUPPORTS_WARNING and MOZ_CXX_SUPPORTS_WARNING to test if the warning is supported before exempting it.
Filing this bug on switching our current exemptions to use that strategy.
Assignee | ||
Comment 1•11 years ago
|
||
I think this should do it.
Assignee | ||
Comment 2•11 years ago
|
||
Actually, this is marginally more straightforward. My previous patch had:
MOZ_CXX_SUPPORTS_WARNING(-Wno-, error=
and this new patch has:
MOZ_CXX_SUPPORTS_WARNING(-W, no-error=
During configuration, we do a test compile with "-W{the second arg}", and if it that succeeds, we go ahead and stick {first arg}{second arg} into _WARNINGS_CXXFLAGS. A few existing clients of this macro try to be cute and stick the "-Wno-" as the first argument, and I cargo-culted that pattern, but I don't think it buys us anything here. We might as well try to actually build with e.g. "-Wno-error=uninitialized" (instead of with "-Werror=uninitialized") in our trial build, as a more-accurate test of whether it's going to succeed in our real build.
Assignee | ||
Comment 3•11 years ago
|
||
Try run: https://tbpl.mozilla.org/?tree=Try&rev=2f907df238ba
I'll request review if that goes well.
Comment 4•11 years ago
|
||
Perhaps I'm missing something... MOZ_C_SUPPORTS_WARNING directly modifies _WARNINGS_CFLAGS. So for the GNUC case you're no longer using WARNINGS_AS_ERRORS. But WARNINGS_AS_ERRORS is still used for other compilers. The inconsistency is unfortunate.
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #4)
> So for the GNUC case you're no longer using
> WARNINGS_AS_ERRORS.
Partly correct. You're right that I'm no longer using it *for the purpose of exempting specific warnings* from being treated as errors anymore.
But I *am* using it for its original purpose -- to store the compiler-specific "warnings-as-errors" flag ("-Werror" in this case).
> But WARNINGS_AS_ERRORS is still used for other
> compilers. The inconsistency is unfortunate.
It's still used to store the compiler-specific "warnings-as-errors" flag, yes, for GCC and for other compilers (MSVC in particular).
Are you saying it's also still used for *exempting* warnings? I don't think it is... (or at least, I'm not seeing that from some quick MXR searching.
Anyway -- I'm missing the inconsistency you're referring to. (or perhaps you were right that you were missing something, and now I've cleared things up? :) )
Assignee | ||
Comment 6•11 years ago
|
||
(One slight self-criticism of my current patch: it'll make us build with these "-Wno-error=uninitialized" etc. compile flags even in non-FAIL_ON_WARNINGS directories, and in non-warnings-as-errors builds, because the patch ends up directly modifying _WARNINGS_{CFLAGS|CXXFLAGS} which get used everywhere.
I think the only way to get a more targeted usage of these flags would be to append them to WARNINGS_AS_ERRORS, as we currently do. So we'd need a modified version of MOZ_C_SUPPORTS_WARNING that targets that variable instead of _WARNINGS_CFLAGS.
Having said that, I'm not particularly worried about this; these flags should have zero effect in non-warnings-as-errors builds/directories, so it's not harmful to have them there. And it makes some sense to conceptually group them with the other warnings that we disable in _WARNINGS_CFLAGS.)
Assignee | ||
Updated•11 years ago
|
Attachment #788434 -
Flags: review?(mh+mozilla)
Comment 7•11 years ago
|
||
Comment on attachment 788434 [details] [diff] [review]
fix v2
Review of attachment 788434 [details] [diff] [review]:
-----------------------------------------------------------------
::: configure.in
@@ +1335,5 @@
> + # -Wdeprecated-declarations - we don't want our builds held hostage when a
> + # platform-specific API becomes deprecated.
> + #
> + MOZ_C_SUPPORTS_WARNING(-W, no-error=uninitialized, ac_c_has_noerror_uninitialized)
> + MOZ_C_SUPPORTS_WARNING(-W, no-error=deprecated-declarations, ac_c_has_noerror_deprecated_declarations)
Maybe only do that when MOZ_ENABLE_WARNINGS_AS_ERRORS is set? (which requires moving the block defining it)
Attachment #788434 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 8•11 years ago
|
||
Good call; that basically addresses my self-criticism in comment 6. I'll fix that, give it a sanity Try run, and land this if all looks good.
Assignee | ||
Comment 9•11 years ago
|
||
OK, here's an updated patch with the MOZ_C*_SUPPORTS_WARNING() statements in an "elif" clause after we check for the mozconfig flag.
Requesting review again, to be on the safe side.
Attachment #788425 -
Attachment is obsolete: true
Attachment #788434 -
Attachment is obsolete: true
Attachment #789377 -
Flags: review?(mh+mozilla)
Updated•11 years ago
|
Attachment #789377 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 10•11 years ago
|
||
Thanks!
I verified locally that FAIL_ON_WARNINGS still gets enforced, and that these targeted "no-error" requests are honored.
Linux/Mac/Win try run: https://tbpl.mozilla.org/?tree=Try&rev=c36d9ae78bf1
Assignee | ||
Comment 11•11 years ago
|
||
Flags: in-testsuite-
Comment 12•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•