1233297 - Reduce duplication in warnings setup code

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nicholas Nethercote [inactive]]

We have four places where compiler warnings are setup:

- configure.in: C and C++ warnings

- js/src/configure.in: C and C++ warnings

All four locations are extremely similar and so most warning changes need to happen in all four places. This is easy to forget and so they tend to get out of sync. We should find a way to put most of the logic in a single place.

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: part-1-generate-MOZ_CXX_SUPPORTS_WARNING-var-name.patch

Part 1: Dynamically generate cached var name for MOZ_C_SUPPORTS_WARNING and MOZ_CXX_SUPPORTS_WARNING.

This patch changes configure's MOZ_C_SUPPORTS_WARNING and MOZ_CXX_SUPPORTS_WARNING functions to dynamically generate the cached var name. This is a prerequisite for reaching the warning names from a separate file.

AC_CACHE_CHECK correctly saves and restores the dynamically-generated cached var name, but the one hitch I haven't figured out how to get AC_CACHE_CHECK to correctly *print* the dynamically-generated cached *result*. MOZ_C_SUPPORTS_WARNING and AC_CACHE_CHECK expand to:

> echo "configure:8200: checking whether the C compiler supports -Werror=sometimes-uninitialized" >&5
> if eval "test \"`echo '$''{'${cv_name}'+set}'`\" = set"; then

which correctly expands to '${ac_cv_c_has_werror_sometimes_uninitialized+set}' but later AC_CACHE_CHECK tries to log the result:

> echo "$ac_t""$${cv_name}" 1>&6

which expands to the "$$" var and "{cv_name}" string instead of dereferencing ${!cv_name} 's value of "yes" or "no":

> checking whether the C++ compiler supports -Werror=sometimes-uninitialized... 88497{cv_name}

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: part-2-extract-warning-flags-from-configure.in.patch

WIP Part 2: Extract warning flags from configure.in into response files.

FIXME: The GLOBAL_DEPS change in config/rules.mk does not seem to force a rebuild if I touch the build/warnings/*flags files. To test changes to the input flags files, I had to disable --with-ccache because the warnings flags come from an generated response file and AFAICT are not understood by ccache. So changing the response files would force a rebuild.

The warnings.cflags and warnings.cxxflags files could be consolidated, but I left them separate. Some warnings are reported in C code xor C++ code, so we wouldn't be able to enable them if the cflags and cxxflags were in a single input file.

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: part-3-enable-more-warnings.patch

WIP Part 3: Enable many more clang and gcc warnings.

I added all the warning flags I found in clang and gcc documentation that are not reported in mozilla-central code. There is a lot of redundancy because I explicitly listed warnings enabled by meta-flags like -Wall. I wanted to create files of all known warning flags without people worrying about which are included in clang's or gcc's -Wall in version X or Y. Just throw them all in one big file. :)

https://gcc.gnu.org/onlinedocs/gcc-4.9.0/gcc/Warning-Options.html
https://gcc.gnu.org/onlinedocs/gcc-4.9.0/gcc/C_002b_002b-Dialect-Options.html
https://github.com/llvm-mirror/clang/blob/master/include/clang/Basic/DiagnosticGroups.td
http://fuckingclangwarnings.com/

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: Weverything.patch

FOR TESTING ONLY: Enable clang -Weverything and opt-out with -Wno-whatever.

clang's -Weverything meta-flag really does enable *EVERYTHING*, including contradictory warnings. This patch enables -Weverything and then opts out of warnings reported in mozilla-central, in theory, resulting in zero reported warnings. This approach is not viable, however, because clang may add experimental new warnings that we don't want to break us.

Here is an informative explanation of -Wall and -Weverything from a clang developer:

http://programmers.stackexchange.com/questions/122608/clang-warning-flags-for-objective-c-development/124574#124574

Comment 5

[Nicholas Nethercote [inactive]]

When I (briefly) looked into doing this I was planning on putting the lists into compiler-opts.m4, which seems simpler than having separate files.

Comment 6

[Chris Peterson [:cpeterson]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> When I (briefly) looked into doing this I was planning on putting the lists
> into compiler-opts.m4, which seems simpler than having separate files.

That sounds easier. I see now that I was trying to solve two problems at the same time, which added unnecessary complications:

1. Consolidate redundant warnings code for CFLAGS and CXXFLAGS in configure.in and js/src/configure.in.
2. Remove MOZ_C[XX]_SUPPORTS_WARNING boilerplate.

Comment 7

[Chris Peterson [:cpeterson]]

Attachment: part-1-consolidate-warning-checks.patch

Part 1: Consolidate MOZ_*_SUPPORTS_WARNING configure checks in compiler-opts.m4.

This patch does not add or remove any warning flags. It just moves the duplicated _WARNINGS_CFLAGS and _WARNINGS_CXXFLAGS code from configure.in and js/src/configure.in to compiler-opts.m4.

Comment 8

[Chris Peterson [:cpeterson]]

Attachment: part-2-enable-more-warnings.patch

Part 2: Enable some more warnings that are not reported in mozilla-central. Personally, I think the comments explaining each warning are redundant and make the code harder to read, but I will continue to follow the existing coding style. :)

-Wreturn-type - catches control reaching end of non-void function. -Werror=return-type was originally in bug 529441, but was lost during some refactoring of configure.in. Only enable for _WARNINGS_CFLAGS because there is actually a -Wreturn-type warning in a C++ function function (which I will fix soon). 

-Wunreachable-code - catches some dead code. This flag is supported by both clang and gcc, though gcc's flag appears to be a no-op.

-Wthread-safety - catches inconsistent use of mutexes (e.g. bug 1136004)

-Wc++1[14z]-compat[-pedantic] - catches C++ version forward-compat issues, e.g. C++1z deprecates the `register` keyword and incrementing bools, so report these issues now.

-Winit-self - catches x=x assignments

-Wwrite-strings - catches treating string literals as non-const

-Wloop-analysis expands -Wrange-loop-analysis to cover more checks

And remove some warning suppressions that are no longer needed:

-Wunreachable-code-aggressive and -Wunreachable-code-return (temporarily) because there are some instances of these warnings that weren't caught because a bug in the MOZ_C*_SUPPORTS_WARNING configure check did incorrectly detect these flags. I will fix that soon.

-Wsometimes-initialized is included in -Wuninitialized, which is included in -Wall

-Wcast-align was only enabled for clang and ICC on architectures other than hppa, ia64, sparc, arm (so only x86?)

-Wno-c++0x-extensions is no longer necessary.

-Wno-extended-offsetof is no longer necessary.

Comment 9

[Chris Peterson [:cpeterson]]

Attachment: part-3-Wno-multichar.patch

Part 3: Android's Stagefright headers pollute many cpp files with its multichar enum values. Consolidate -Wno-multichar warning suppressions from 14 different moz.build to one line in compiler-opts.m4.

Comment 10

[Chris Peterson [:cpeterson]]

Attachment: part-4-remove-unnecessary-warning-suppressions.patch

Part 4: Remove unneeded warning suppressions from third-party directories that ALLOW_COMPILER_WARNINGS. This patch removes some ugly code from many moz.build files whose only purpose was to hide third-party warnings from mach's warnings-summary output. Now that warnings-as-errors is enabled by default for first-party code, mach's warnings-summary doesn't need to be as tidy because warning regressions will be caught as errors.

Comment 11

[Chris Peterson [:cpeterson]]

Attachment: part-5_remove-TRIMMED-and-TRACING.patch

Part 5: Remove TRIMMED and TRACING macros. They appear to be unused, so we can remove them to shrink our compiler flags.

https://mxr.mozilla.org/mozilla-central/search?string=[^A-Z_]TRIMMED[^A-Z_]&regexp=1&find=&findi=&filter=TRIMMED&hitlimit=&tree=mozilla-central

https://mxr.mozilla.org/mozilla-central/search?string=[^A-Z_]TRACING[^A-Z_]&regexp=1&find=&findi=&filter=TRACING&hitlimit=&tree=mozilla-central

These macros landed back in 1999 and, AFAICT, they were unused even then!

https://github.com/mozilla/gecko-dev/commit/ed0937533edc6e35c0ea01761af4ce28719e55ee#diff-3b3a6ec97232deb43dc14319a73872c1R2321

Also remove MOZ_DEBUG_ENABLE_DEFS because it is only used to initialize MOZ_DEBUG_DEFINES and move the debug-label loop into the MOZ_DEBUG code path.

Comment 12

[Nicholas Nethercote [inactive]]

> -Wreturn-type - catches control reaching end of non-void function.
> -Werror=return-type was originally in bug 529441, but was lost during some
> refactoring of configure.in. Only enable for _WARNINGS_CFLAGS because there
> is actually a -Wreturn-type warning in a C++ function function (which I will
> fix soon). 

-Wreturn-type is enabled as part of -Wall. I don't think it's worth making it always fatal because if you use --enable-warnings-as-errors (as happens on automation) it'll be fatal anyway.

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Nicholas Nethercote [:njn] from comment #12)
> -Wreturn-type is enabled as part of -Wall. I don't think it's worth making
> it always fatal because if you use --enable-warnings-as-errors (as happens
> on automation) it'll be fatal anyway.

The upside of making it always-fatal is that it lets developers catch a common mistake (forgetting a return statement) quickly & locally, with zero false positives, without needing to know about a special mozconfig flag.

Comment 14

[Chris Peterson [:cpeterson]]

(In reply to Nicholas Nethercote [:njn] from comment #12)
> -Wreturn-type is enabled as part of -Wall. I don't think it's worth making
> it always fatal because if you use --enable-warnings-as-errors (as happens
> on automation) it'll be fatal anyway.

OK.

Also, it also looks like -Wno-unused-local-typedef is no longer necessary. This is the clang flag; gcc's flag is -Wno-unused-local-typedefs, with an 's'. Suppressing gcc's warning was recently deemed unnecessary and RESOLVED WONTFIX in bug 1243604, so it's unsurprising that we no longer need it on clang either.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37c913668202

Comment 15

[Nicholas Nethercote [inactive]]

> The upside of making it always-fatal is that it lets developers catch a
> common mistake (forgetting a return statement) quickly & locally, with zero
> false positives, without needing to know about a special mozconfig flag.

Or they can just use --enable-warnings-as-errors, which will give them that *plus* they'll also get immediate feedback about all the other kinds of errors that'll show up later on TreeHerder (be it try or inbound). Anyone using a reasonably normal configuration should really have --enable-warnings-as-errors on.

Comment 16

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #15)
> > The upside of making it always-fatal is that it lets developers catch a
> > common mistake (forgetting a return statement) quickly & locally, with zero
> > false positives, without needing to know about a special mozconfig flag.
> 
> Or they can just use --enable-warnings-as-errors, which will give them that
> *plus* they'll also get immediate feedback about all the other kinds of
> errors that'll show up later on TreeHerder (be it try or inbound). Anyone
> using a reasonably normal configuration should really have
> --enable-warnings-as-errors on.

... depends what you means with "reasonably normal configuration". If you're using a compiler more recent than automation, you're very likely to blow up on warnings that are *not* present on automation in code that you didn't change.

Comment 17

[Mike Hommey [:glandium]]

Comment on attachment 8719132 [details] [diff] [review]
part-2-enable-more-warnings.patch

Review of attachment 8719132 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/autoconf/compiler-opts.m4
@@ +426,5 @@
>  
>  AC_DEFUN([MOZ_SET_WARNINGS_CFLAGS],
>  [
>      # Turn on gcc/clang warnings:
> +    # https://gcc.gnu.org/onlinedocs/gcc-4.9.0/gcc/Warning-Options.html

We may or may not switch to gcc 4.9, but even if we do so, we will still support building with gcc 4.8. And we haven't switched off 4.7 anyways at the moment, so you can't assume the flags from 4.9 are supported.

@@ +439,3 @@
>      _WARNINGS_CFLAGS="${_WARNINGS_CFLAGS} -Wall"
>      _WARNINGS_CFLAGS="${_WARNINGS_CFLAGS} -Wempty-body"
> +    _WARNINGS_CFLAGS="${_WARNINGS_CFLAGS} -Werror=return-type"

More than the discussion about it being good or not for local builds, this makes third-party code use -Werror=return-type even when ALLOW_COMPILER_WARNINGS is set, which can lead to fun problems. So no, we really shouldn't have -Werror flags in WARNING_C*FLAGS.

@@ -457,5 @@
>      if test "$MOZ_ENABLE_WARNINGS_AS_ERRORS"; then
>          MOZ_C_SUPPORTS_WARNING(-Werror=, non-literal-null-conversion, ac_c_has_non_literal_null_conversion)
>      fi
> -    MOZ_C_SUPPORTS_WARNING(-W, sometimes-uninitialized, ac_c_has_sometimes_uninitialized)
> -    MOZ_C_SUPPORTS_WARNING(-W, unreachable-code-aggressive, ac_c_has_wunreachable_code_aggressive)

Why remove these two?

@@ +475,5 @@
>      #   platform-specific API becomes deprecated.
>      # -Wfree-nonheap-object - false positives during PGO
>      # -Warray-bounds - false positives depending on optimization
> +    _WARNINGS_CFLAGS="${_WARNINGS_CFLAGS} -Wno-error=array-bounds"
> +    _WARNINGS_CFLAGS="${_WARNINGS_CFLAGS} -Wno-error=deprecated-declarations"

why?

Comment 18

[Mike Hommey [:glandium]]

Comment on attachment 8719133 [details] [diff] [review]
part-3-Wno-multichar.patch

Review of attachment 8719133 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/autoconf/compiler-opts.m4
@@ +547,2 @@
>      _WARNINGS_CXXFLAGS="${_WARNINGS_CXXFLAGS} -Wno-invalid-offsetof"
> +    _WARNINGS_CXXFLAGS="${_WARNINGS_CXXFLAGS} -Wno-multichar"

Why would we want to disable this globally?

Comment 19

[Mike Hommey [:glandium]]

Comment on attachment 8719135 [details] [diff] [review]
part-4-remove-unnecessary-warning-suppressions.patch

Review of attachment 8719135 [details] [diff] [review]:
-----------------------------------------------------------------

> Now that warnings-as-errors is enabled by default for first-party code

Not for local builds

> mach's warnings-summary doesn't need to be as tidy because warning regressions will be caught as errors.

mach's warnings-summary is only useful for local builds. This change would make mach's warnings-summary useless unless it filters out warnings in directories that have ALLOW_COMPILER_WARNINGS, which, afaik, it doesn't.

::: build/autoconf/android.m4
@@ -152,5 @@
>      OBJCOPY="$android_toolchain"/bin/"$android_tool_prefix"-objcopy
>  
>      CPPFLAGS="-idirafter $android_platform/usr/include $CPPFLAGS"
>      CFLAGS="-mandroid -fno-short-enums -fno-exceptions $CFLAGS"
> -    CXXFLAGS="-mandroid -fno-short-enums -fno-exceptions -Wno-psabi $CXXFLAGS"

Is bug 760335 not true anymore?

Comment 20

[Mike Hommey [:glandium]]

Comment on attachment 8719136 [details] [diff] [review]
part-5_remove-TRIMMED-and-TRACING.patch

Review of attachment 8719136 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/autoconf/compiler-opts.m4
@@ -127,5 @@
>  fi
>  
>  AC_SUBST(MOZ_NO_DEBUG_RTL)
>  
> -MOZ_DEBUG_ENABLE_DEFS="DEBUG TRACING"

I'm pretty sure -DTRACING and -DTRIMMED are for system headers, not for anything in the tree. Whether they still do something in 2016, I don't know, but we can't say they don't just because they don't appear in the tree.

@@ -134,5 @@
> -                          Define DEBUG_<value> for each comma-separated
> -                          value given.],
> -[ for option in `echo $withval | sed 's/,/ /g'`; do
> -    MOZ_DEBUG_ENABLE_DEFS="$MOZ_DEBUG_ENABLE_DEFS DEBUG_${option}"
> -done])

With the way autoconf works, moving this block is weird.

Comment 21

[Chris Peterson [:cpeterson]]

(In reply to Mike Hommey [:glandium] from comment #17)
> Comment on attachment 8719132 [details] [diff] [review]
> >      # Turn on gcc/clang warnings:
> > +    # https://gcc.gnu.org/onlinedocs/gcc-4.9.0/gcc/Warning-Options.html
> 
> We may or may not switch to gcc 4.9, but even if we do so, we will still
> support building with gcc 4.8. And we haven't switched off 4.7 anyways at
> the moment, so you can't assume the flags from 4.9 are supported.

OK. Since people can use gcc 4.8 or 4.9, those flags could be optionally enabled with MOZ_CXX_SUPPORTS_WARNING. But since we aren't enabling any of them today, I will leave the gcc 4.7 URL.


> @@ +439,3 @@
> >      _WARNINGS_CFLAGS="${_WARNINGS_CFLAGS} -Wall"
> >      _WARNINGS_CFLAGS="${_WARNINGS_CFLAGS} -Wempty-body"
> > +    _WARNINGS_CFLAGS="${_WARNINGS_CFLAGS} -Werror=return-type"
> 
> More than the discussion about it being good or not for local builds, this
> makes third-party code use -Werror=return-type even when
> ALLOW_COMPILER_WARNINGS is set, which can lead to fun problems. So no, we
> really shouldn't have -Werror flags in WARNING_C*FLAGS.

OK. SGTM.


> @@ -457,5 @@
> >      if test "$MOZ_ENABLE_WARNINGS_AS_ERRORS"; then
> >          MOZ_C_SUPPORTS_WARNING(-Werror=, non-literal-null-conversion, ac_c_has_non_literal_null_conversion)
> >      fi
> > -    MOZ_C_SUPPORTS_WARNING(-W, sometimes-uninitialized, ac_c_has_sometimes_uninitialized)
> > -    MOZ_C_SUPPORTS_WARNING(-W, unreachable-code-aggressive, ac_c_has_wunreachable_code_aggressive)
> 
> Why remove these two?

clang's -Wsometimes-initialized is already included in -Wuninitialized, which is included in -Wall

These MOZ_C*_SUPPORTS_WARNING checks for -Wunreachable-code-aggressive (which includes -Wunreachable-code-return) are broken due to, ironically, a -Wunreachable-code-return warning about an extra (and unreachable) return(0) in the code MOZ_C*_SUPPORTS_WARNING compiles with AC_TRY_COMPILE:

https://hg.mozilla.org/mozilla-central/file/tip/build/autoconf/compiler-opts.m4#l396

When I originally fixed the last -Wunreachable-code warnings (bug 1235717), I had been enabling the flags in my mozconfig, so I did not realize the configure checks were broken. There have been a couple -Wunreachable-code-return regressions since. I think we should remove the broken configure checks until the new warnings have been fixed. (I have local patches for some.)


> @@ +475,5 @@
> >      #   platform-specific API becomes deprecated.
> >      # -Wfree-nonheap-object - false positives during PGO
> >      # -Warray-bounds - false positives depending on optimization
> > +    _WARNINGS_CFLAGS="${_WARNINGS_CFLAGS} -Wno-error=array-bounds"
> > +    _WARNINGS_CFLAGS="${_WARNINGS_CFLAGS} -Wno-error=deprecated-declarations"
> 
> why?

Since both clang and gcc support -Warray-bounds and -Wdeprecated-declarations, I thought this code would be simplifer than the existing MOZ_C_SUPPORTS_WARNING checks for -Wno-error=array-bounds and -Wno-error=deprecated-declarations (which are removed in the patch).


(In reply to Mike Hommey [:glandium] from comment #18)
> Comment on attachment 8719133 [details] [diff] [review]
> > +    _WARNINGS_CXXFLAGS="${_WARNINGS_CXXFLAGS} -Wno-multichar"
> 
> Why would we want to disable this globally?

Android's Stagefright headers pollute many cpp files with its multichar enum values like enum { E = 'Fooo'; }. This warning does not seem very useful to me, yet it we need to suppress it in 14 different moz.build files (mostly related to Android and Firefox OS). Replacing those 14 suppressions with one line in compiler-opts.m4 seems like a fair trade-off to me, but I don't have a strong opinion.


(In reply to Mike Hommey [:glandium] from comment #19)
> > mach's warnings-summary doesn't need to be as tidy because warning regressions will be caught as errors.
> 
> mach's warnings-summary is only useful for local builds. This change would
> make mach's warnings-summary useless unless it filters out warnings in
> directories that have ALLOW_COMPILER_WARNINGS, which, afaik, it doesn't.

Yes, I see what you mean. I had been submitting patches to suppress warnings in third-party libraries, but some module owners have rejected them because the warnings are not errors (thanks to ALLOW_COMPILER_WARNINGS). Suppressing them all will cause some churn in moz.build files as library code is updated or compilers add new warning flags. But without suppressing them, then, as you point out, mach warnings-summary is useless. Though, if we only care about warnings in non-ALLOW_COMPILER_WARNINGS directories, then mach warnings-summary is unnecessary because any warnings would be errors.


> ::: build/autoconf/android.m4
> @@ -152,5 @@
> >      OBJCOPY="$android_toolchain"/bin/"$android_tool_prefix"-objcopy
> >  
> >      CPPFLAGS="-idirafter $android_platform/usr/include $CPPFLAGS"
> >      CFLAGS="-mandroid -fno-short-enums -fno-exceptions $CFLAGS"
> > -    CXXFLAGS="-mandroid -fno-short-enums -fno-exceptions -Wno-psabi $CXXFLAGS"
> 
> Is bug 760335 not true anymore?

Correct. -Wno-psabi has not been necessary since we updated the build machines to Android NDK r8c three years ago. I submitted a patch to remove -Wno-psabi back then (in bug 826133), but you pointed out that developers could still be using an older NDK for their local builds. I think we can safely assume that all Android developers should update to an NDK version newer than r8c (November 2012). :-)


(In reply to Mike Hommey [:glandium] from comment #20)
> Comment on attachment 8719136 [details] [diff] [review]
> > -MOZ_DEBUG_ENABLE_DEFS="DEBUG TRACING"
> 
> I'm pretty sure -DTRACING and -DTRIMMED are for system headers, not for
> anything in the tree. Whether they still do something in 2016, I don't know,
> but we can't say they don't just because they don't appear in the tree.

OK. Google found a couple references to "ifdef TRIMMED" and "ifdef TRACING", but none of them look like system headers. I can leave these macro definitions. I just thought it would be an opportunity to reduce the (growing) number of compiler flags.


> @@ -134,5 @@
> > -                          Define DEBUG_<value> for each comma-separated
> > -                          value given.],
> > -[ for option in `echo $withval | sed 's/,/ /g'`; do
> > -    MOZ_DEBUG_ENABLE_DEFS="$MOZ_DEBUG_ENABLE_DEFS DEBUG_${option}"
> > -done])
> 
> With the way autoconf works, moving this block is weird.

OK. I will revert that change.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15621f98b53b

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/15621f98b53b

Comment 24

[Nicholas Nethercote [inactive]]

The landed patch avoids the duplication between configure.in and js/src/configure.in, yay!

But there is still a lot of duplication between the C warnings code and the C++ warnings code in compiler-opts.m4. cpeterson, do you plan to remove that duplication? Thanks.

Comment 25

[Chris Peterson [:cpeterson]]

(In reply to Nicholas Nethercote [:njn] from comment #24)
> But there is still a lot of duplication between the C warnings code and the
> C++ warnings code in compiler-opts.m4. cpeterson, do you plan to remove that
> duplication? Thanks.

I wasn't sure how to approach that. Perhaps a unified MOZ_SET_WARNINGS_FLAGS function could take an out-parameter variable name (_WARNINGS_CFLAGS or _WARNINGS_CXXFLAGS) that can be passed through to a unified MOZ_C*_SUPPORTS_WARNING function.

I'll take a look. If you have any suggestions, just let me know. I still think it would be nice if all these compiler flags could be extracted into @response files. That seems tidier and would reduce the number of command line arguments.

Comment 26

[Mike Hommey [:glandium]]

Don't spend too much time on this. This part of configure is likely to completely change in the coming weeks.

Comment 27

[Chris Peterson [:cpeterson]]

Attachment: part-2a-enable-more-warnings.patch

Part 2a: Enable some more warnings

Update patch #2 and split into two separate patches to make the intent clearer: one patch to enable more warning flags and a second to remove unnecessary flags.

Comment 28

[Chris Peterson [:cpeterson]]

Comment on attachment 8721761 [details] [diff] [review]
part-2a-enable-more-warnings.patch

Review of attachment 8721761 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/autoconf/compiler-opts.m4
@@ +523,3 @@
>      _WARNINGS_CXXFLAGS="${_WARNINGS_CXXFLAGS} -Woverloaded-virtual"
>      _WARNINGS_CXXFLAGS="${_WARNINGS_CXXFLAGS} -Wpointer-arith"
> +    _WARNINGS_CXXFLAGS="${_WARNINGS_CXXFLAGS} -Wsign-compare"

Because -Wsign-compare is enabled by gcc's -Wall (for C++ only), but not by clang's -Wall! -Wsign-compare is enabled for C and C++ by gcc's -Wextra and clang's -Wextra.

Comment 29

[Chris Peterson [:cpeterson]]

Attachment: part-2b-remove-some-warnings.patch

Part 2b: Remove some unnecessary warning flags

Comment 30

[Chris Peterson [:cpeterson]]

Comment on attachment 8721762 [details] [diff] [review]
part-2b-remove-some-warnings.patch

Review of attachment 8721762 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/autoconf/android.m4
@@ +152,5 @@
>      OBJCOPY="$android_toolchain"/bin/"$android_tool_prefix"-objcopy
>  
>      CPPFLAGS="-idirafter $android_platform/usr/include $CPPFLAGS"
>      CFLAGS="-mandroid -fno-short-enums -fno-exceptions $CFLAGS"
> +    CXXFLAGS="-mandroid -fno-short-enums -fno-exceptions $CXXFLAGS"

-Wno-psabi has not been necessary since we updated the build machines to Android NDK r8c three years ago.

::: build/autoconf/compiler-opts.m4
@@ +461,5 @@
>  
>      if test "$MOZ_ENABLE_WARNINGS_AS_ERRORS"; then
>          MOZ_C_SUPPORTS_WARNING(-Werror=, non-literal-null-conversion, ac_c_has_non_literal_null_conversion)
>      fi
> +

-Wsometimes-initialized is not necessary because it is implicitly enabled by clang's -Wuninitialized, which is enable by -Wall.

@@ -469,5 @@
> -    # -Wcast-align - catches problems with cast alignment
> -    if test -z "$INTEL_CC" -a -z "$CLANG_CC"; then
> -       # Don't use -Wcast-align with ICC or clang
> -       case "$CPU_ARCH" in
> -           # And don't use it on hppa, ia64, sparc, arm, since it's noisy there

-Wcast-align is very noisy and we apparently only enable it for gcc on 32-bit x86 builds?

@@ -482,4 @@
>      # Turn off some non-useful warnings that -Wall turns on.
>  
> -    # -Wno-unused-local-typedef - catches unused typedefs, which are commonly used in assertion macros
> -    MOZ_C_SUPPORTS_WARNING(-Wno-, unused-local-typedef, ac_c_has_wno_unused_local_typedef)

-Wno-unused-local-typedef is no longer necessary. This is the clang flag; gcc's flag is -Wno-unused-local-typedefs, with an 's'. Suppressing gcc's warning was recently deemed unnecessary and WONTFIX'd in bug 1243604. Unsurprisingly, we no longer need it on clang either.

@@ -529,5 @@
>      # -Wclass-varargs - catches objects passed by value to variadic functions.
>      # -Wimplicit-fallthrough - catches unintentional fallthroughs in switch cases
>      # -Wloop-analysis - catches issues around loops
>      # -Wnon-literal-null-conversion - catches expressions used as a null pointer constant
> -    # -Wrange-loop-analysis - catches copies during range-based for loops.

-Wrange-loop-analysis is no longer necessary because it is implicitly enabled by -Wloop-analysis, which is enabled by patch #2a.

@@ -590,5 @@
> -    # we use it to opt out of the warning, enabling (macro-encapsulated) use of
> -    # deleted function syntax.
> -    if test "$CLANG_CXX"; then
> -        _WARNINGS_CXXFLAGS="${_WARNINGS_CXXFLAGS} -Wno-c++0x-extensions"
> -        MOZ_CXX_SUPPORTS_WARNING(-Wno-, extended-offsetof, ac_cxx_has_wno_extended_offsetof)

These clang warning suppressions are no longer necessary.

Comment 31

[Mike Hommey [:glandium]]

Comment on attachment 8721762 [details] [diff] [review]
part-2b-remove-some-warnings.patch

Review of attachment 8721762 [details] [diff] [review]:
-----------------------------------------------------------------

The comments about why things are removed are valuable, you should put them in the commit message.

Comment 32

[Chris Peterson [:cpeterson]]

Thanks, Mike. I have copied my patch comments into my commit message.

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a64eaf8d688
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2bbe8e91ea2

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7a64eaf8d688
https://hg.mozilla.org/mozilla-central/rev/a2bbe8e91ea2