Closed
Bug 1272513
Opened 9 years ago
Closed 6 years ago
Enable -Wshadow warnings
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
INCOMPLETE
People
(Reporter: cpeterson, Assigned: cpeterson)
References
Details
Attachments
(2 files)
76.73 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
97.76 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
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.
Attachment #8751960 -
Flags: review?(mh+mozilla)
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.
Attachment #8751967 -
Flags: review?(mh+mozilla)
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.
Attachment #8751960 -
Flags: review?(mh+mozilla) → review+
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
Attachment #8751967 -
Flags: review?(mh+mozilla) → review+
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.
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.
Keywords: leave-open
Comment 7•9 years ago
|
||
bugherder |
Assignee | ||
Updated•9 years ago
|
Updated•7 years ago
|
Product: Core → Firefox Build System
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?
Flags: needinfo?(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®exp=true
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(cpeterson)
Keywords: leave-open
Resolution: --- → WONTFIX
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.
Alias: Wshadow
Resolution: WONTFIX → INCOMPLETE
You need to log in
before you can comment on or make changes to this bug.
Description
•