Closed Bug 857863 Opened 11 years ago Closed 11 years ago

Disable MSVC warning C4244 ('conversion' conversion from 'type1' to 'type2', possible loss of data), since it's making us exempt MSVC from FAIL_ON_WARNINGS all over the place

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla23

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file)

As shown by the third patch on bug 824247, we have a lot of directories that are FAIL_ON_WARNINGS-except-for-MSVC.

As khuey pointed out in a dev.platform post today, this causes pain when a patch builds fine locally but introduces a warning in one of these directories & ends up busting our non-MSVC builds.  If we could get rid of these MSVC exemptions, then our devs who use MSVC would be better-able to catch & address those sorts of issues locally, without burning TBPL.

As I suggested at the end of my response...
  https://groups.google.com/d/msg/mozilla.dev.platform/j-eyeYt9MPg/C7bR_IJ6yDkJ
...I suspect the best way forward is to audit the warnings that MSVC reports for those directories, and treat the spammiest/false-positive-est warnings as warnings even in FAIL_ON_WARNINGS contexts. (assuming MSVC lets us do that)  Then, hopefully, we'll be able to remove the MSVC exemptions, and still report the warnings but not error out on those specific ones.
For reference, bug 716541 is one instance where we did something like this for a GCC warning that had a lot of false positives.

For reference, the "tweak MSVC warnings" chunk of configure.in is here:
https://mxr.mozilla.org/mozilla-central/source/configure.in?rev=81d4bd15053d&mark=2138-2151#2138
I'm actually not sure MSVC supports exactly what I'm talking about.  It looks like you can disable specific warnings completely, but I don't know if you can "neuter" them as I'd like to do.

Maybe disabling these warnings completely would be OK... I hate having to resort to that, though.

Documentation on this is here:
 http://msdn.microsoft.com/en-us/library/thxezb7y.aspx
(In reply to comment #2)
> Maybe disabling these warnings completely would be OK... I hate having to
> resort to that, though.

Why?
Because it feels like a cop-out, for warnings that might point to real bugs (or at least code that's asking for cleanup, like bug 825949). It's nice to at least give the compiler a chance to tell you about it, without necessarily holding ourselves to fixing the many existing false-positive-instances.

Having said that, I don't know the full list of warnings in question; they might be useless enough to just silence.
So I don't think there's a way to disable warnings-as-errors for specific warnings in VC++. I think we have two possible options here:
a) Use -wdNNNN to disable specific spammy warnings.
b) Use -weNNNN to make specific really bad warnings into errors.
(In reply to Daniel Holbert [:dholbert] from comment #4)
> Having said that, I don't know the full list of warnings in question; they
> might be useless enough to just silence.

See bug 824247 comment 12 & 13 (I've mix-up the bug number, sorry).
So the top 3 warnings there are:
C4244 - 'conversion' conversion from 'type1' to 'type2', possible loss of data
C4018 - 'expression' : signed/unsigned mismatch
C4355 - 'this' : used in base member initializer list

We have 1702(!) instances of C4244, that's probably not tractable. All the other warnings in the list have < 50 occurances, and are probably approachable.
(In reply to comment #7)
> So the top 3 warnings there are:
> C4244 - 'conversion' conversion from 'type1' to 'type2', possible loss of data

This is actually a very useful warning at least for some of the types.  I fixed a bug where we were clobbering the high word of a 64-bit number because it was implicitly cast to a 32-bit value a few weeks ago...

> C4018 - 'expression' : signed/unsigned mismatch

This we have enabled on gcc and clang.  Not sure how useful it is in practice.

> C4355 - 'this' : used in base member initializer list

This is actually an indicator of a bug in most cases.

> We have 1702(!) instances of C4244, that's probably not tractable. All the
> other warnings in the list have < 50 occurances, and are probably approachable.

The high number of C4244 makes me really sad.  Perhaps MSVC is just more aggressive than gcc is about implicit type conversion warnings?
GCC doesn't warn for implicit type conversion a
[sorry, cat hit my tab key]
GCC doesn't warn for implicit type conversion at all, by default (with -Wall), IIRC. See:
 http://gcc.gnu.org/wiki/NewWconversion#Frequently_Asked_Questions
Ok, so -wd4244 seems like a no-brainer then.
Summary: Make the spammiest MSVC warnings always be treated as warnings (the ones exempting it from FAIL_ON_WARNINGS all over the place) → Disable/neuter the spammiest MSVC warnings (the ones exempting MSVC from FAIL_ON_WARNINGS all over the place)
Turns out we already have -wd4244 in js/src/configure.in. :)

Patch coming up to disable it for the toplevel configure.in, too.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #733425 - Flags: review?(ted)
(The patch also shifts the "khuey says we can safely ignore MSVC warning C4251" comment a few lines, to preserve the numeric ordering (by warning ID) of the explanatory comments.)
Attachment #733425 - Flags: review?(ted) → review+
I verified locally that the patch successfully disables the warning. With the patch applied, we still get 26 instances of C4244, but they're all in nss / nsprpub, since those are built with their own compile options.
Blocks: 858224
[Clarifying bug summary. Per comment 7, 8, & 10, it seems pretty clear that C4244 is by far the main culprit here. If we end up needing to fiddle with other warnings, we can do so in another bug.]

I filed bug 858224 on removing the MSVC exemptions from FAIL_ON_WARNINGS, once this is fixed. (and addressing any hopefully-minor fallout from that)
Summary: Disable/neuter the spammiest MSVC warnings (the ones exempting MSVC from FAIL_ON_WARNINGS all over the place) → Disable MSVC warning C4244, since it's making us exempt MSVC from FAIL_ON_WARNINGS all over the place
Summary: Disable MSVC warning C4244, since it's making us exempt MSVC from FAIL_ON_WARNINGS all over the place → Disable MSVC warning C4244 ('conversion' conversion from 'type1' to 'type2', possible loss of data), since it's making us exempt MSVC from FAIL_ON_WARNINGS all over the place
https://hg.mozilla.org/mozilla-central/rev/6f832cf6485f
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Blocks: 851306
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: