Don't enable tree-wide -Werror=* warnings as errors without --enable-warnings-as-errors

RESOLVED FIXED in mozilla36

Status

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

unspecified
mozilla36
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Part 1: Only enable tree-wide -Werror=* warnings-as-errors with --enable-warnings-as-errors opt-in.

Move all tree-wide -Werror=* flags (e.g. bug 1076698) behind an --enable-warnings-as-errors check. This gives people (such as downstream projects like comm-central) an escape from warnings-as-errors for their code or particular compiler configuration.
Attachment #8512481 - Flags: review?(mh+mozilla)
Part 2: Add -Werror=parentheses and -Werror=switch behind --enable-warnings-as-errors check.

With patch #1's warnings-as-errors escape hatch, we can add back -Werror=parentheses and -Werror=switch that were backed out for breaking bz (bug 1081208) and comm-central (bug 1080351), respectively.

Here is a green Try build with the above patches:

https://tbpl.mozilla.org/?tree=Try&rev=1fc669872c59
Attachment #8512485 - Flags: review?(mh+mozilla)
Attachment #8512481 - Flags: review?(mh+mozilla) → review+
Attachment #8512485 - Flags: review?(mh+mozilla) → review+
Is it worth making an exception for Werror=return-type? In my experience, it's the only warning that has both (a) a zero false positive rate, and (b) reasonable code never violates it.
[marking comments 2-6 as obsolete, since they were based on me mixing up -Wparentheses with -Wparentheses-equality]

(In reply to Nicholas Nethercote [:njn] from comment #7)
> Is it worth making an exception for Werror=return-type? In my experience,
> it's the only warning that has both (a) a zero false positive rate, and (b)
> reasonable code never violates it.

Good point -- we should probably consider making that one always-fatal, along with the other already-globally-Werror-even-before-bug 1076698 warnings. (which were: int-to-pointer-cast, and -Wtype-limits just in C++, according to the removed chunks in https://hg.mozilla.org/integration/mozilla-inbound/rev/22558583014e#l1.12 )
(In reply to Daniel Holbert [:dholbert] from comment #8)
> (In reply to Nicholas Nethercote [:njn] from comment #7)
> > Is it worth making an exception for Werror=return-type? In my experience,
> > it's the only warning that has both (a) a zero false positive rate, and (b)
> > reasonable code never violates it.
> 
> Good point -- we should probably consider making that one always-fatal,
> along with the other already-globally-Werror-even-before-bug 1076698
> warnings. (which were: int-to-pointer-cast, and -Wtype-limits just in C++,
> according to the removed chunks in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/22558583014e#l1.12 )

I considered that, but went with the all-or-nothing approach. In fact, after a couple comm-central fixes, all the -Werror=* warnings I added recently have are working fine. The only -Werror=* warnings that caused problems (and were backed out) were -Wparentheses and -Wswitch, so just those two could be landed behind the --enable-warnings-as-errors checks. I think that is preferable because it also sneaks some warnings-as-errors into B2G builds (whose mozconfigs purposely exclude --enable-warnings-as-errors for some reason). :)

I can submit an alternate patch for Mike's consideration.
Blocks: 1086705
No longer blocks: 1080351
This combined patch is an alternative approach that only adds just -Wparentheses and -Wswitch behind the --enable-warnings-as-errors checks.

SpiderMonkey's -Wswitch doesn't need to be behind --enable-warnings-as-errors because the comm-central's -Wswitch problem was caused by their extensions to the nsresult enum, which are not probably not relevant to SpiderMonkey.
Attachment #8514077 - Flags: review?(mh+mozilla)
Comment on attachment 8514077 [details] [diff] [review]
option-2-some-warnings-as-errors.patch

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

::: configure.in
@@ +1576,5 @@
> +    # Treat some warnings as errors if --enable-warnings-as-errors:
> +    if test "$MOZ_ENABLE_WARNINGS_AS_ERRORS"; then
> +        _WARNINGS_CXXFLAGS="${_WARNINGS_CXXFLAGS} -Werror=parentheses"
> +        _WARNINGS_CXXFLAGS="${_WARNINGS_CXXFLAGS} -Werror=switch"
> +    fi

The point of enable-warnings-as-errors is that it just adds -Werror. Adding more -Werror=foo does not really make sense.
Except enable-warnings-as-error *doesn't* add -Werror to directories without FAIL_ON_WARNINGS = True, and with that change it would add those -Werror=foo for those directories.
I'm not sure we want to go down this road.
Attachment #8514077 - Flags: review?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #12)
> The point of enable-warnings-as-errors is that it just adds -Werror. Adding
> more -Werror=foo does not really make sense.
> Except enable-warnings-as-error *doesn't* add -Werror to directories without
> FAIL_ON_WARNINGS = True, and with that change it would add those -Werror=foo
> for those directories.
> I'm not sure we want to go down this road.

That makes sense: warnings are warnings unless --enable-warnings-as-errors makes them errors. :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/54be5416ae5d
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc4b163f7db7
https://hg.mozilla.org/mozilla-central/rev/54be5416ae5d
https://hg.mozilla.org/mozilla-central/rev/dc4b163f7db7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.