Closed Bug 824247 Opened 7 years ago Closed 7 years ago

Enable warnings as errors (with FAIL_ON_WARNINGS) for MSVC

Categories

(Firefox Build System :: General, defect)

All
Windows 8
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla20

People

(Reporter: emk, Assigned: emk)

References

Details

Attachments

(5 files, 2 obsolete files)

I think we need somthing like FAIL_ON_WARNINGS_MSVC because MSVC is generating too many warnings on FAIL_ON_WARNINGS directories right now.
Blocks: 703121
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #695482 - Flags: review?(ted)
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
Attachment #695483 - Flags: review?(bugs) → review+
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.
Attachment #695483 - Attachment description: Part 2: Fix common source of warning spam (dom) → Part 1: Fix common source of warning spam (dom)
Attachment #695484 - Attachment description: Part 3: Fix common source of warning span (dom/bindings) → Part 2: Fix common source of warning span (dom/bindings)
(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
Attachment #695482 - Attachment is obsolete: true
Attachment #695485 - Attachment is obsolete: true
Attachment #695482 - Flags: review?(ted)
Attachment #695485 - Flags: review?(ted)
Attachment #696290 - Flags: superreview?(ted)
Attachment #696290 - Flags: superreview?(ted) → superreview+
Summary: Enable WARNINGS_AS_ERRORS for MSVC → Enable warnings as errors (with FAIL_ON_WARNINGS) for MSVC
(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.
Depends on: 826597
Depends on: 827643
(Sorry for the Japanese localized messages)
Attached file Warnings summary
./mach warnings-list and ./mach warnings-summary did the great job.
No longer blocks: FAIL_ON_WARNINGS
Depends on: FAIL_ON_WARNINGS
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.