In configure.in warnings are set up like this: enable -Wall enable -W1 # not part of -Wall if MOZ_ENABLE_WARNINGS_AS_ERRORS enable -Werror=2 fi The |if| block makes no sense. - If -W2 is part of -Wall, we don't need to enable -Werror=2 when MOZ_ENABLE_WARNINGS_AS_ERRORS is true, because -Werror (or equivalent) will do that anyway. - If -W2 is not part of -Wall, then we should turn it on all the time, not only when MOZ_ENABLE_WARNINGS_AS_ERRORS is true. The simpler and better way to do this would be like so: enable -Wall enable -W1 enable -W2 # but only if -W2 is not part of -Wall One consequence of doing it in this streamlined is that we won't have any -Werror-foo switches used in ALLOW_COMPILER_WARNINGS directories, which is a good thing; to do otherwise makes ALLOW_COMPILER_WARNINGS behave unexpectedly. This problem was introduced in bug 1090088, and glandium even complained about it in one of the patches in that bug that didn't land; see bug 1090088 comment 12. https://bugzilla.mozilla.org/show_bug.cgi?id=1090088#c12 but r+'d part 1 in that bug, which did something similar. - anyway, time to fix it

I will also take this opportunity to make the C and C++ warnings as similar as possible, and also to make configure.in and js/src/configure.in identical.

(In reply to Nicholas Nethercote [:njn] from comment #0) > - If -W2 is part of -Wall, we don't need to enable -Werror=2 when > MOZ_ENABLE_WARNINGS_AS_ERRORS is true, because -Werror (or equivalent) > will do that anyway. My goal was to enable -Werror for warnings that were not reported in any directories, including directories that could not be fully FAIL_ON_WARNINGS because they had other warnings. This was a "two-dimensional" attack on warnings: 1. Some directories had no warnings and could be FAIL_ON_WARNINGS. 2. Some warnings were in no directories and could be -Werror in all directories. I agree that this configure.in code no longer makes sense now that we have enabled warnings-as-errors by default.

The main changes are to the warnings, which are as follows. - Kept -Wall. - Part of -Wall or on by default; remove all mentions: * -Waddress * -Wchar-subscripts * -Wcomment * -Wconversion-null * -Wendif-labels * -Wenum-compare * -Wimplicit-function-declaration * -Wint-to-pointer-cast * -Wmissing-braces * -Wmultichar * -Wnonnull * -Wparentheses * -Wpointer-sign * -Wpointer-to-int-cast (C only) * -Wreorder * -Wreturn-type * -Wsequence-point * -Wsign-compare (C++ only) * -Wtrigraphs * -Wuninitialized * -Wunknown-pragmas * -Wunused-label * -Wunused-value * -Wwrite-strings (C++ only) - Part of -Wextra; kept where present, added where missing: * -Wempty-body * -Wignored-qualifiers (not added for C++ code; many fixes needed to enable) * -Wtype-limits - Part of -pedantic; kept where present, added where missing: * -pointer-arith - C++ only, kept: * -Wno-invalid-offsetof * -Woverloaded-virtual - Clang-only, kept: * -Wnon-literal-null-conversion (affected by a clang bug; see the big comment in the code) * -Wrange-loop-analysis (C++ only) * -Wno-unused-local-typedef - This no longer exists? I kept it to be safe: * -Winline-new-delete Other changes: - Some C warnings incorrectly used the CXX macros. Fixes that. - Moves comments about warnings closer to the lines where they are defined, to make it easier to keep the comments consistent with the code. - Reorders things a little, e.g. so that all enabled warnings are before all disabled warnings. The C and C++ warnings are now very similar, in both configure.in and js/src/configure.in.

We *really* need to find a way to refactor this code so that these changes don't have to be made in four different places each time. This patch is a precursor to that, because it minimizes the differences between the four places.

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=088223487eb7&selectedJob=14683067

(In reply to Nicholas Nethercote [:njn] from comment #4) > We *really* need to find a way to refactor this code so that these changes > don't have to be made in four different places each time. This patch is a > precursor to that, because it minimizes the differences between the four > places. I have some WIP patches that consolidate all the warning configure code and move the list of -W flags into a separate text file.

> I have some WIP patches that consolidate all the warning configure code and > move the list of -W flags into a separate text file. Can you post them in bug 1233297? The patches here are likely to clash horribly with anything you've done. I was going to do the refactoring via compiler-opts.m4 but if you've already started via a different approach I'm happy to let you continue.

Comment on attachment 8699328 [details] [diff] [review] Streamline setting of compile warnings in configure.in Review of attachment 8699328 [details] [diff] [review]: ----------------------------------------------------------------- LGTM! ::: configure.in @@ +1440,1 @@ > # -Wtype-limits - catches overflow bugs, few false positives I think we should remove all these warning comments, but I don't care too much. The comments double the size of the _WARNINGS_CFLAGS code blocks. If someone wants to know more about a warning, they can read the documentation on gcc.gnu.org or fuckingclangwarnings.com. @@ +1562,5 @@ > + _WARNINGS_CXXFLAGS="${_WARNINGS_CXXFLAGS} -Wno-invalid-offsetof" > + > + # -Wno-inline-new-delete - we inline 'new' and 'delete' in mozalloc > + # -Wno-unused-local-typedef - catches unused typedefs, which are commonly used in assertion macros > + MOZ_CXX_SUPPORTS_WARNING(-Wno-, inline-new-delete, ac_cxx_has_wno_inline_new_delete) As you noted, -Winline-new-delete no longer seems to exist. There is no reference to it in the clang mirror on GitHub, yet clang on OS X silently accepts -Winline-new-delete. https://github.com/llvm-mirror/clang/search?utf8=%E2%9C%93&q=inline-new-delete

(In reply to Nicholas Nethercote [:njn] from comment #5) > Try looks good: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=088223487eb7&selectedJob=14683067 Whoops, https://treeherder.mozilla.org/#/jobs?repo=try&revision=c81ff1711bd6 is better because it has all platforms rather than just OS X.

Comment on attachment 8699328 [details] [diff] [review] Streamline setting of compile warnings in configure.in Review of attachment 8699328 [details] [diff] [review]: ----------------------------------------------------------------- One thing this does change is that for directories with ALLOW_COMPILER_WARNINGS, we currently have all the -Werror=... flags that you're removing. It makes sense that we don't keep them, but it's worth mentioning in the commit message. (Typo in the commit message: -pointer-arith instead of -Wpointer-arith) ::: configure.in @@ -1448,5 @@ > - # -Wpointer-sign - catches mixing pointers to signed and unsigned types > - # -Wpointer-to-int-cast - catches casts from pointer to different sized int > - # -Wreturn-type - catches missing returns, zero false positives > - # -Wsequence-point - catches undefined order behavior like `a = a++` > - # -Wsign-compare - catches comparison of signed and unsigned types As you mention in the commit message, -Wsign-compare is in -Wall for C++ only. We were setting it in both WARNINGS_CFLAGS and WARNING_CXXFLAGS, so you're effectively removing it from CFLAGS, which you shouldn't. @@ -1549,5 @@ > - # -Wrange-loop-analysis - catches copies during range-based for loops. > - # -Wreturn-type - catches missing returns, zero false positives > - # -Wsequence-point - catches undefined order behavior like `a = a++` > - # -Wsign-compare - catches comparison of signed and unsigned types > - # -Wswitch - catches switches without all enum cases or default case You didn't mention this one in your commit message, but it's part of -Wall. ::: js/src/configure.in @@ +1199,3 @@ > # -Wnon-literal-null-conversion - catches expressions used as a null pointer constant > + # -Wsometimes-initialized - catches some uninitialized values > + MOZ_C_SUPPORTS_WARNING(-W, non-literal-null-conversion, ac_c_has_non_literal_null_conversion) Might be worth mentioning that this is not a problem somehow in js code, contrary to gecko? @@ +1268,3 @@ > # -Wrange-loop-analysis - catches copies during range-based for loops. > + # -Wsometimes-initialized - catches some uninitialized values > + MOZ_CXX_SUPPORTS_WARNING(-W, non-literal-null-conversion, ac_cxx_has_non_literal_null_conversion) Likewise

> ::: js/src/configure.in > @@ +1199,3 @@ > > # -Wnon-literal-null-conversion - catches expressions used as a null pointer constant > > + # -Wsometimes-initialized - catches some uninitialized values > > + MOZ_C_SUPPORTS_WARNING(-W, non-literal-null-conversion, ac_c_has_non_literal_null_conversion) > > Might be worth mentioning that this is not a problem somehow in js code, > contrary to gecko? I'll just put the workaround into js/src/configure.in too. That will make factoring out this code easier.

> I think we should remove all these warning comments, but I don't care too much. I ended up keeping them. It's useful to have explanations for the ones added beyond -Wall, and the ones turned off from -Wall. And once the duplication is factored out there won't be so much repetition.

https://hg.mozilla.org/integration/mozilla-inbound/rev/470ab2722530f603fde64de3554cbf16b39787db Bug 1232224 - Streamline setting of compile warnings in configure.in. r=glandium,cpeterson.