Closed Bug 1207030 Opened 4 years ago Closed 4 years ago

Enable -Wshadow flag in more directories that have no -Wshadow warnings

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

Details

Attachments

(1 file)

-Wshadow warnings in ipc/chromium headers were transitively affecting many unrelated directories. Now that they have been fixed in bug 1203232, more directories are now free of -Wshadow warnings.

Here is a green try build enabling the -Wshadow flag in more directories:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97211bf91c1b
Attachment #8664061 - Flags: review?(mh+mozilla)
Comment on attachment 8664061 [details] [diff] [review]
part-2-more-Wshadow-directories.patch

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

::: xpcom/threads/moz.build
@@ +83,5 @@
>  
>  include('/ipc/chromium/chromium-config.mozbuild')
> +
> +if CONFIG['GNU_CXX']:
> +    CXXFLAGS += ['-Wshadow']

+1 for this one :)
Depends on: 1207031
Comment on attachment 8664061 [details] [diff] [review]
part-2-more-Wshadow-directories.patch

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

is the "part 2" in the patch name a mistake, or is there a missing part 1?

this brings the count of manually added -Wshadow from 55 to 232. This is still under the crude estimate of the number of directories with sources (which `git grep -l '\b\(UNIFIED_SOURCES\|SOURCES\)' -- \*/moz.build \*/\*mozbuild` places it at 570), but it is close enough that we probably have to start thinking how to deal with it in a nice way.
Attachment #8664061 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #2)
> is the "part 2" in the patch name a mistake, or is there a missing part 1?

I should have removed "part 2" from the name. Part 1 is an XPCOM -Wshadow fix in bug 1207031 that I need to land first.


> this brings the count of manually added -Wshadow from 55 to 232. This is
> still under the crude estimate of the number of directories with sources
> (which `git grep -l '\b\(UNIFIED_SOURCES\|SOURCES\)' -- \*/moz.build
> \*/\*mozbuild` places it at 570), but it is close enough that we probably
> have to start thinking how to deal with it in a nice way.

Good point. This patch is probably the tipping point where I should think about another way forward. I could reverse the logic, enabling -Wshadow in configure.in and opting out with -Wno-shadow in individual directories.

Do you have a different approach in mind? I also considered packaging the CONFIG/CXXFLAGS checks into a macro like `FAIL_ON_SHADOW_WARNINGS = True` or `ALLOW_SHADOW_WARNINGS = True`. That would reduce some of the redundant copy/paste code.
I'm not really a big fan of specialized moz.build variables. We'll end up multiplying them, and that's not necessarily a good situation to end in. I guess -Wno-shadow would be a good starting point, although I've been pondering about something like -= support (as in CXXFLAGS -= ['-Wshadow']), but that would require more thought and work.
https://hg.mozilla.org/mozilla-central/rev/20a95408d007
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Blocks: 1272513
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.