Chris Peterson [:cpeterson] Assignee
	Description • 9 years ago

Add -Wno-error=shadow CXXFLAGS to moz.build files in directories that still have -Wshadow warnings, so their warnings will be reported by not treated as compilation errors.

I suppressed reporting of -Wshadow warnings entirely (-Wno-shadow) in the js/src directories. SpiderMonkey's naming conventions permit parameter names to shadow member variable names (e.g. constructor parameters and initializer lists), so they produce a lot of build noise.

This patch adds -Wno-error=shadow to about 160 moz.build files, but the next patch removes -Wshadow from over 260 moz.build files. The net result is fewer lines of code in fewer moz.build files while enabling -Wshadow in more directories.

	Chris Peterson [:cpeterson] Assignee
	Comment 1 • 9 years ago

Part 2: Enable -Wshadow warnings and remove redundant -Wshadow CXXFLAGS from moz.build files.

Patch #2 can't land until I fix -Wshadow warnings in some common header files that would otherwise require -Wno-error=shadow suppressions in moz.build files for many unrelated directories.

	Mike Hommey [:glandium]
	Comment 2 • 9 years ago

Comment on attachment 8751960 [details] [diff] [review]
part-1-Wno-error-shadow.patch

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

rs=me. I think it would be better to have the same flags on clang and gcc. That they don't emit the same shadowing warnings seems weird to me, and sound like a cause of future pain if we go with asymmetric flags.

::: accessible/ipc/moz.build
@@ +48,5 @@
>      FINAL_LIBRARY = 'xul'
>  
>  include('/ipc/chromium/chromium-config.mozbuild')
> +
> +if CONFIG['GNU_CXX'] and not CONFIG['CLANG_CXX']:

Funny. But doesn't that risk breaking clang builds on hypothetical future clang updates?

::: dom/camera/moz.build
@@ +72,5 @@
>  #  - about attributes on forward declarations for types that are already
>  #    defined, which complains about an important MOZ_EXPORT for android::AString
>  if CONFIG['GNU_CC']:
> +    CXXFLAGS += ['-Wno-error=attributes']
> +    if CONFIG['CLANG_CXX']:

O_o

::: security/pkix/test/gtest/moz.build
@@ +40,5 @@
>  # required to implement tests, and/or because they originate in the GTest
>  # framework in a way we cannot otherwise work around.
> +if CONFIG['GNU_CXX']:
> +    CXXFLAGS += ['-Wno-error=shadow']
> +    if CONFIG['CLANG_CXX']:

No need for the extra indentation here.

	Mike Hommey [:glandium]
	Comment 3 • 9 years ago

Comment on attachment 8751967 [details] [diff] [review]
part-2-Wshadow.patch

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

rs=me

	Chris Peterson [:cpeterson] Assignee
	Comment 4 • 9 years ago

(In reply to Mike Hommey [:glandium] from comment #2)
> rs=me. I think it would be better to have the same flags on clang and gcc.
> That they don't emit the same shadowing warnings seems weird to me, and
> sound like a cause of future pain if we go with asymmetric flags.

OK. I can change the patches to use the same flags on clang and gcc.

Since clang's and gcc's -Wshadow flags can warn about different code, some directories are warning-free in one compiler but the other. My thinking was it would be good to retain partial -Wshadow coverage for one compiler until the other compiler's warnings are also fixed.


> > +if CONFIG['GNU_CXX'] and not CONFIG['CLANG_CXX']:
> 
> Funny. But doesn't that risk breaking clang builds on hypothetical future
> clang updates?

Sure, but wouldn't that be the case for clang or gcc in any directory without -Wno-error=shadow? I guess that depends on my assumption above that enabling warnings in one compiler is better than enabling them in none.


> > +if CONFIG['GNU_CXX']:
> > +    CXXFLAGS += ['-Wno-error=shadow']
> > +    if CONFIG['CLANG_CXX']:
> 
> No need for the extra indentation here.

OK. This moz.build file inconsistently uses 4-space and 2-space indentation, but I can remove the indentation fix in this change.

	Pulsebot
	Comment 5 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/2572c0758dd4
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ee18d7a875a

	Chris Peterson [:cpeterson] Assignee
	Comment 6 • 9 years ago

I updated the moz.build file changes to use the same -Wno-error=shadow warnings for clang and gcc. I will land the change to actually enable -Wshadow warnings in compiler-opts.m4 after I've fixed the blocking bugs.

	Phil Ringnalda (:philor)
	Comment 7 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/2572c0758dd4
https://hg.mozilla.org/mozilla-central/rev/0ee18d7a875a

	BugBot [:suhaib / :marco/ :calixte]
	Comment 8 • 6 years ago

The leave-open keyword is there and there is no activity for 6 months.
:cpeterson, maybe it's time to close this bug?

	Chris Peterson [:cpeterson] Assignee
	Comment 9 • 6 years ago

Here is a summary of how many different -Wshadow warnings we still have in mozilla-central:

1484 -Wshadow warnings
 502 -Wshadow-field-in-constructor warnings
 117 -Wshadow-uncaptured-local
  61 -Wshadow-field warnings
   0 -Wshadow-field-in-constructor-modified warnings. This warning is already enabled by bug 1483761.
   0 -Wshadow-ivar warnings. This warning is enabled by default in clang.

https://clang.llvm.org/docs/DiagnosticsReference.html#wshadow

I'm resolving this bug as WONTFIX because enabling -Wshadow warnings is not practical at this time. There are just too many to warnings to fix. However, fixing and enabling -Wshadow-field warnings might be doable because there are only 61.

I should probably remove the -Wno-shadow and -Wno-error=shadow warning suppressions added to moz.build files in patch 1:

https://searchfox.org/mozilla-central/search?q=Wno-(error%3D)%3Fshadow&case=true&regexp=true

	Chris Peterson [:cpeterson] Assignee
	Comment 10 • 6 years ago

Correction: after I removed the -Wno-shadow warning suppressions added to moz.build files in patch 1:

1860 -Wshadow (but "only" about 900 in first-party Mozilla code)
1107 -Wshadow-field-in-constructor
 179 -Wshadow-uncaptured-local
  61 -Wshadow-field

I originally enabled -Wshadow warnings in warning-free directories (in bug 1198124). In this bug, I intended to flip the logic by enabling -Wshadow warnings globally and suppressing the warnings in affected directories' moz.build files. Unfortunately, issues prevented -Wshadow from being enabled globally. Leaving the code from bug 1198124 would have at least held the line against -Wshadow regressions.

Maybe I will look into switching back to that logic. I randomly checked a few instances of -Wshadow warnings and they definitely point to dodgy or confusing code that would be nice to fix.