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. On 04/03/2013 11:48 AM, Kyle Huey wrote: > (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 > _______________________________________________ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform > _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
