858224 - Remove the MSVC exemptions for FAIL_ON_WARNINGS that are safe to remove, now that C4244 is disabled

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Daniel Holbert [:dholbert]]

Now that we're disabling our spammiest MSVC warning (C4244) in bug 857863, we should be able to remove many of the MSVC exemptions for FAIL_ON_WARNINGS.

Rough list of these exemptions (with a few unrelated hits that use the same "_MSC_VER" check):
https://mxr.mozilla.org/mozilla-central/search?string=ifdef+_MSC_VER&find=Makefile.in&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

I'm optimistically filing this bug on removing all of the exemptions, though that may require a few spot-fixes of other warnings here and there.

Comment 1

[Daniel Holbert [:dholbert]]

emk, any chance you're interested in taking this? I can take it, too, but I might not get to it right away.

Comment 2

[Masatoshi Kimura [:emk]]

Will do.

Comment 3

[Masatoshi Kimura [:emk]]

Actually FAIL_ON_WARNINGS was wrapped by if*n*def _MSC_VER :) https://mxr.mozilla.org/mozilla-central/search?string=ifndef+_MSC_VER&find=Makefile.in&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 4

[Daniel Holbert [:dholbert]]

oops, right :) that's what I *meant* to search for in Comment 0.

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: patch

I'll file follow-up bugs for directories which require changes other than Makefile.in.

Comment 6

[Masatoshi Kimura [:emk]]

https://tbpl.mozilla.org/?tree=Try&rev=db26bca23038

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 734248 [details] [diff] [review]
patch

Looks good to me! One nit on the commit message:

>Bug 858224 - Remove the MSVC exemptions for FAIL_ON_WARNINGS

That sounds like you're removing all of the exemptions, which you're not (yet).

So -- say "Remove most" instead of "the"), since some will remain (temporarily) after this patch lands.

Also, technically a widescale Makefile tweak like this should get build peer review (though I'm sure it'll be granted, and likely quickly). Hence, kicking review over to ted.

Comment 8

[Daniel Holbert [:dholbert]]

(Thanks very much for taking care of these, BTW!)

Comment 9

[Daniel Holbert [:dholbert]]

CC'ing khuey, so he can steal this review if he sees it before Ted.

Comment 10

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d80e4fd84bb4

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d80e4fd84bb4