The current state of WARNINGS_AS_ERRORS is untenable

[Kyle Huey]

(Ignoring whether or not WARNINGS_AS_ERRORS is a good idea, ignoring
whether or not having it disabled by default on developer builds but
enabled on automation is a good idea, ignoring whether or not we can deal
with the tendency of gcc to lurch from complaining incessantly about one
silly thing to complaining about another on version upgrades, ignoring the
differences in warnings between different compilers ..)

The lack of platform parity for WARNINGS_AS_ERRORS being enabled at all
makes developing on Windows a complete PITA. There are 50+ directories[0]
that have WARNINGS_AS_ERRORS enabled for gcc/clang and not MSVC. So I come
along and write a patch that builds just fine on my machine, and push it to
inbound and every other platform is broken. Now I have to push pretty much
every patch I write to try (or I just have to disable WARNINGS_AS_ERRORS in
directories where problems occur).

Either this parity issue needs to be fixed or we need to turn off
WARNINGS_AS_ERRORS.

- Kyle

[0] http://hg.mozilla.org/mozilla-central/rev/4983b42d58c9

[Daniel Holbert]

Stepping back: [
This issue is really a special case of "This patch compiles fine in my
local configuration, but it busts the build for $OTHER_PLATFORM".

The general solution to this class of problems is a try push, with
builds on at least one platform other than your local config (if not all
platforms). That should catch the vast majority of these cases (both
warning-related and non-warning-related), and I think it's reasonable to
expect that non-trivial patches should generally receive a try push
along those lines before landing.
]

Now -- for the specific point you brought up about MSVC being excluded
from FAIL_ON_WARNINGS in some directories -- that indeed is undesirable,
and makes it much easier to hit this sort of thing for non-try-tested
pushes. (Usually the MSVC exemption is there because of MSVC-only
warnings for double --> float or float --> int conversions, which we
apparently run afoul of all over the place :-/ )

I absolutely agree we should do something to address those and remove
the MSVC-special-casing there. Ideally we'd fix the code so it doesn't
warn, but that's not easy and maybe not necessary in many cases where
this warning might be a false-positive.

IMHO, the sanest way to address this specific issue would be to use
compiler flags to force the spammiest MSVC-only warnings to be treated
as warnings (not errors), even in FAIL_ON_WARNINGS directories. We
already do this for at least one GCC warning (-Wuninitialized, which has
a lot of false positives.)

With this change, we could remove most/all of the MSVC exemptions and
get MSVC build-warning-coverage in those directories, so you'd be better
able to locally catch future instances of this sort of issue.

Would that address your concerns?

~Daniel

P.S. Just to provide the relevant keywords, for MXR searchability &
completeness for anyone who's missing the context here:
* --enable-warnings-as-errors is the mozconfig option that will make
your build treat warnings as errors, for any opted-in directories.
* FAIL_ON_WARNINGS is the Makefile variable used to opt-in directories.

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>

[Daniel Holbert]

MSVC C4244 is by far our spammiest MSVC warning -- it's already disabled
in /js/src due to its low value::trouble ratio. Once the tree reopens,
I'll be disabling it in the toplevel configure.in -- more info on that here:
https://bugzilla.mozilla.org/show_bug.cgi?id=857863
(There doesn't seem to be a way to keep it enabled but force it to be
treated as a warning, as I'd suggested in my previous post, so we're
just disabling it.)

At that point, we'll hopefully be able to address the MSVC
FAIL_ON_WARNINGS parity issues, here:
https://bugzilla.mozilla.org/show_bug.cgi?id=858224

~Daniel

[Jeff Walden]

On 04/04/2013 01:55 PM, Daniel Holbert wrote:
> MSVC C4244 is by far our spammiest MSVC warning -- it's already disabled
> in /js/src due to its low value::trouble ratio.

Actually I think this is a mistake, one that we should fix at some point. I have very little confidence that all of SpiderMonkey's float->narrower type conversions are at all correct, and that we're not stripping off information in the process. But I haven't had time to investigate them and correct all the existing mistakes, tho, to move forward here.

Jeff

[Nicholas Nethercote]

On Thu, Apr 4, 2013 at 4:50 PM, Jeff Walden <jwald...@mit.edu> wrote:
> On 04/04/2013 01:55 PM, Daniel Holbert wrote:
>> MSVC C4244 is by far our spammiest MSVC warning -- it's already disabled
>> in /js/src due to its low value::trouble ratio.
>
> Actually I think this is a mistake, one that we should fix at some point.

Bug 857863 says there are 1702 occurrences of this warning in Gecko
(excluding SpiderMonkey).
Warnings about possibly dubious but common practices aren't much use.
I gave up on turning on -Wshadow for this reason, and it looks like
C4244 has a higher false positive rate. It'll be an uphill battle at
best.

Nick

[Karl Tomlinson]

Nicholas Nethercote writes:

> Warnings about possibly dubious but common practices aren't much use.
> I gave up on turning on -Wshadow for this reason,

If you could now use |index| as a variable name, would it be worth
revisiting -Wshadow?

http://gcc.gnu.org/gcc-4.8/changes.html

[Nicholas Nethercote]

That's a big improvement, and IIUC it puts GCC 4.8 on par with clang.
But even then there are still hundreds of warnings. Read the gory
details at https://bugzilla.mozilla.org/show_bug.cgi?id=563195#c14 and
subsequent comments.

Nick