824247 - Enable warnings as errors (with FAIL_ON_WARNINGS) for MSVC

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Masatoshi Kimura [:emk]]

I think we need somthing like FAIL_ON_WARNINGS_MSVC because MSVC is generating too many warnings on FAIL_ON_WARNINGS directories right now.

Comment 1

[Honza Bambas (:mayhemer)]

+1

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Introduce FAIL_ON_WARNINGS_MSVC

Comment 3

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Fix common source of warning spam (dom)

Comment 4

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Fix common source of warning span (dom/bindings)

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: Part 4: Enable FAIL_ON_WARNINGS_MSVC on already warning-free directories on MSVC

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 695483 [details] [diff] [review]
Part 1: Fix common source of warning spam (dom)

> 
>   nsresult InitTimer(nsTimerCallbackFunc aFunc, uint64_t delay) {
>-    return mTimer->InitWithFuncCallback(aFunc, this, delay,
>+    return mTimer->InitWithFuncCallback(aFunc, this,
>+                                        static_cast<uint32_t>(delay),
>                                         nsITimer::TYPE_ONE_SHOT);

Hmm, odd, why does InitTimer take uint64_t

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 695482 [details] [diff] [review]
Part 1: Introduce FAIL_ON_WARNINGS_MSVC

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

Why not just wrap FAIL_ON_WARNINGS in "ifndef _MSC_VER" in directories where you don't want it to apply for MSVC? Seems a lot simpler than introducing this new variable.

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: Part 3: Disable FAIL_ON_WARNINGS on not-yet-warning-free directories on MSVC

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> Why not just wrap FAIL_ON_WARNINGS in "ifndef _MSC_VER" in directories where
> you don't want it to apply for MSVC? Seems a lot simpler than introducing
> this new variable.

Fair enough.
I had to move some lines because _MSC_VER was not available until autoconf.mk is included.

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

Comment 9

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e626106842f
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a4639f9ed38
https://hg.mozilla.org/integration/mozilla-inbound/rev/4983b42d58c9

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Olli Pettay [:smaug] from comment #6)
> Comment on attachment 695483 [details] [diff] [review]
> Part 1: Fix common source of warning spam (dom)
> 
> > 
> >   nsresult InitTimer(nsTimerCallbackFunc aFunc, uint64_t delay) {
> >-    return mTimer->InitWithFuncCallback(aFunc, this, delay,
> >+    return mTimer->InitWithFuncCallback(aFunc, this,
> >+                                        static_cast<uint32_t>(delay),
> >                                         nsITimer::TYPE_ONE_SHOT);
> 
> Hmm, odd, why does InitTimer take uint64_t

I noticed this, too -- I think we should make InitTimer take a uint32_t, rather than casting its argument to that.  I filed bug 825949 on that.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0e626106842f
https://hg.mozilla.org/mozilla-central/rev/0a4639f9ed38
https://hg.mozilla.org/mozilla-central/rev/4983b42d58c9

Comment 12

[Masatoshi Kimura [:emk]]

Attachment: All current warnings generated by MSVC

(Sorry for the Japanese localized messages)

Comment 13

[Masatoshi Kimura [:emk]]

Attachment: Warnings summary

./mach warnings-list and ./mach warnings-summary did the great job.