Enable -Wshadow in directories that have no -Wshadow warnings

RESOLVED FIXED in Firefox 43

Status

()

Core
Build Config
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Created attachment 8652176 [details] [diff] [review]
add-Wshadow-to-mozbuild-files.patch

Bug 563195 comment 22 suggested enabling -Wshadow piecemeal in the tree. This patch adds -Wshadow to moz.build files in directories that currently have no -Wshadow warnings.
Attachment #8652176 - Flags: review?(mh+mozilla)
Comment on attachment 8652176 [details] [diff] [review]
add-Wshadow-to-mozbuild-files.patch

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

r+ with the unrelated changes left out, and the layout/base/gtest thing fixed.

::: dom/push/moz.build
@@ +48,5 @@
> +
> +if CONFIG['GNU_CXX']:
> +    CXXFLAGS += ['-Wshadow']
> +
> +FAIL_ON_WARNINGS = True

This change is unrelated, and while I was seeing this, I was wondering how many directories have FAIL_ON_WARNINGS on now. And the answer is 486 as of my tree from last week. Whereas a crude git grep -l SOURCES -- '*/moz.build' '*.mozbuild' say there are 653 files with SOURCES in them (which, incidentally, also matches XPIDL_SOURCES, so that would be an upper bound).
I think it's well about time to turn the default around for this. Would you mind doing this in a separate bug?

::: layout/base/gtest/moz.build
@@ +18,5 @@
>  # Workaround bug 1142396. Suppress the warning from gmock library for clang.
>  if CONFIG['CLANG_CXX']:
> +    CXXFLAGS += [
> +        '-Wno-null-dereference',
> +        '-Wshadow',

That's clang specific branch, that leaves out GCC.

::: memory/replace/jemalloc/moz.build
@@ +3,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +UNIFIED_SOURCES += [

This is unrelated.

@@ +12,5 @@
>  # Android doesn't have pthread_atfork, so just implement a dummy function.
>  # It shouldn't make much problem, as the use of fork is pretty limited on
>  # Android.
>  if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'android':
> +    UNIFIED_SOURCES += [

Likewise.
Attachment #8652176 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 2

3 years ago
Thanks!

(In reply to Mike Hommey [:glandium] from comment #1)
> This change is unrelated, and while I was seeing this, I was wondering how
> many directories have FAIL_ON_WARNINGS on now. And the answer is 486 as of
> my tree from last week. Whereas a crude git grep -l SOURCES -- '*/moz.build'
> '*.mozbuild' say there are 653 files with SOURCES in them (which,
> incidentally, also matches XPIDL_SOURCES, so that would be an upper bound).
> I think it's well about time to turn the default around for this. Would you
> mind doing this in a separate bug?

I filed bug 1198334 to change FAIL_ON_WARNINGS' default value from False to True and fix the mozbuild files.
https://hg.mozilla.org/mozilla-central/rev/fc23dad2b383
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
> https://hg.mozilla.org/mozilla-central/rev/fc23dad2b383

The patch modified only 36 directories. Are there really that few that are shadow-free? All the more reason to start pushing that number up, I guess...
(Assignee)

Comment 6

3 years ago
(In reply to Nicholas Nethercote [:njn] from comment #5)
> The patch modified only 36 directories. Are there really that few that are
> shadow-free? All the more reason to start pushing that number up, I guess...

Yes, but many (most?) of the warnings are from shadow warnings from ipc/chromium/src/base headers indirectly included in .cpp files. Perhaps we can quarantine the chromium headers in a .cpp file and just expose a Mozilla-style API.

But there might be easier wins from some header file trimming. Here is an example -Wshadow warning in widget/gtk from (FFOS's?) NuwaParent.h. From a cursory look at dom/ipc/NuwaParent.h, the header includes chromium's message_loop.h, but AFAICT doesn't actually use any of its declarations!


 17:39:25 INFO - In file included from /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/task.h:11:0,
17:39:25 INFO - from /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/message_loop.h:17,
17:39:25 INFO - from ../../dist/include/mozilla/dom/NuwaParent.h:10,
17:39:25 INFO - from ../../dist/include/mozilla/dom/ContentParent.h:10,
17:39:25 INFO - from /builds/slave/try-l64-d-00000000000000000000/build/src/widget/nsBaseDragService.h:18,
17:39:25 INFO - from /builds/slave/try-l64-d-00000000000000000000/build/src/widget/gtk/nsDragService.h:11,
17:39:25 INFO - from /builds/slave/try-l64-d-00000000000000000000/build/src/widget/gtk/nsDragService.cpp:7,
17:39:25 INFO - from /builds/slave/try-l64-d-00000000000000000000/build/src/obj-firefox/widget/gtk/Unified_cpp_widget_gtk0.cpp:101:
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h: In constructor 'Tuple1<A>::Tuple1(typename TupleTraits<A>::ParamType)': 
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h: In constructor 'Tuple1<A>::Tuple1(typename TupleTraits<A>::ParamType)':
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:81:57: error: declaration of 'a' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h: In constructor 'Tuple2<A, B>::Tuple2(typename TupleTraits<A>::ParamType, typename TupleTraits<B>::ParamType)':
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:99:7: error: declaration of 'b' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:99:7: error: declaration of 'a' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h: In constructor 'Tuple3<A, B, C>::Tuple3(typename TupleTraits<A>::ParamType, typename TupleTraits<B>::ParamType, typename TupleTraits<C>::ParamType)':
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:123:7: error: declaration of 'c' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:123:7: error: declaration of 'b' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:123:7: error: declaration of 'a' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h: In constructor 'Tuple4<A, B, C, D>::Tuple4(typename TupleTraits<A>::ParamType, typename TupleTraits<B>::ParamType, typename TupleTraits<C>::ParamType, typename TupleTraits<D>::ParamType)':
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:152:7: error: declaration of 'd' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:152:7: error: declaration of 'c' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:152:7: error: declaration of 'b' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:152:7: error: declaration of 'a' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h: In constructor 'Tuple5<A, B, C, D, E>::Tuple5(typename TupleTraits<A>::ParamType, typename TupleTraits<B>::ParamType, typename TupleTraits<C>::ParamType, typename TupleTraits<D>::ParamType, typename TupleTraits<E>::ParamType)':
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:186:5: error: declaration of 'e' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:186:5: error: declaration of 'd' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:186:5: error: declaration of 'c' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:186:5: error: declaration of 'b' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:186:5: error: declaration of 'a' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h: In constructor 'Tuple6<A, B, C, D, E, F>::Tuple6(typename TupleTraits<A>::ParamType, typename TupleTraits<B>::ParamType, typename TupleTraits<C>::ParamType, typename TupleTraits<D>::ParamType, typename TupleTraits<E>::ParamType, typename TupleTraits<F>::ParamType)':
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:225:5: error: declaration of 'f' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:225:5: error: declaration of 'e' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:225:5: error: declaration of 'd' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:225:5: error: declaration of 'c' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:225:5: error: declaration of 'b' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:225:5: error: declaration of 'a' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h: In constructor 'Tuple7<A, B, C, D, E, F, G>::Tuple7(typename TupleTraits<A>::ParamType, typename TupleTraits<B>::ParamType, typename TupleTraits<C>::ParamType, typename TupleTraits<D>::ParamType, typename TupleTraits<E>::ParamType, typename TupleTraits<F>::ParamType, typename TupleTraits<G>::ParamType)':
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:269:5: error: declaration of 'g' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:269:5: error: declaration of 'f' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:269:5: error: declaration of 'e' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:269:5: error: declaration of 'd' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:269:5: error: declaration of 'c' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:269:5: error: declaration of 'b' shadows a member of 'this' [-Werror=shadow]
17:39:25 INFO - /builds/slave/try-l64-d-00000000000000000000/build/src/ipc/chromium/src/base/tuple.h:269:5: error: declaration of 'a' shadows a member of 'this' [-Werror=shadow]
(Assignee)

Updated

3 years ago
Blocks: 1207030
(Assignee)

Updated

2 years ago
Blocks: 1272513
You need to log in before you can comment on or make changes to this bug.