Last Comment Bug 716738 - Add FAIL_ON_WARNINGS in layout/tables
: Add FAIL_ON_WARNINGS in layout/tables
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: Trunk
: All Linux
: -- normal (vote)
: mozilla12
Assigned To: Daniel Holbert [:dholbert] (largely AFK until June 28)
:
Mentors:
Depends on: 703121
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-09 16:39 PST by Daniel Holbert [:dholbert] (largely AFK until June 28)
Modified: 2012-01-21 07:10 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (624 bytes, patch)
2012-01-09 16:41 PST, Daniel Holbert [:dholbert] (largely AFK until June 28)
dbaron: review+
Details | Diff | Review

Description Daniel Holbert [:dholbert] (largely AFK until June 28) 2012-01-09 16:39:11 PST
Now that bug 703121 has landed, we can start labeling gcc-warning-free directories as FAIL_ON_WARNINGS, to keep them warning-free.

Filing this bug on adding that label in layout/tables, which is already gcc-warning-free (both opt & debug) on my system. (discounting -Wuninitialized warnings which we're ignoring for the purposes of warnings-as-errors)
Comment 1 Daniel Holbert [:dholbert] (largely AFK until June 28) 2012-01-09 16:41:46 PST
Created attachment 587199 [details] [diff] [review]
patch v1

I've confirmed that this errors out in --enable-warnings-as-errors builds, if I introduce code in layout/tables that causes build warnings. (i.e. this patch does what it's intended to do)

Currently running this through Try (along with bug 716699) as a sanity check:
 https://tbpl.mozilla.org/?tree=Try&rev=5fb8ea0712d7
Comment 2 Daniel Holbert [:dholbert] (largely AFK until June 28) 2012-01-09 17:05:20 PST
Comment on attachment 587199 [details] [diff] [review]
patch v1

Canceling review for now -- it looks like the "-Wno-error=uninitialized" from bug 716541 is ineffective on mac, probably because we use an old version of GCC there (g++-4.2), so we do hit a "may be used uninitialized" warning and treat it as an error on Mac opt:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dholbert@mozilla.com-5fb8ea0712d7/try-macosx-debug/try-macosx-debug-build6451.txt.gz

Waiting on this bug's patch until that's fixed.
Comment 3 Daniel Holbert [:dholbert] (largely AFK until June 28) 2012-01-09 17:11:18 PST
Actually, I might have mis-identified the source of that build failure.

The text "cc1plus: warnings being treated as errors" does appear before some uninitialized-variable warnings, but then there are a bunch of lines like...
> {standard input}:16725:symbol: "L__ZN9nsGkAtoms10tableFrameE$non_lazy_ptr" can't be undefined in a subtraction expression
...and...
> {standard input}:unknown:Undefined local symbol L__ZN9nsGkAtoms16bcTableCellFrameE$non_lazy_ptr
...before the build actually errors out.  So maybe those were the source of the build failure. (I'll investigate some more.)
Comment 4 Daniel Holbert [:dholbert] (largely AFK until June 28) 2012-01-09 17:43:19 PST
Not sure about that earlier build error yet.

One other update though -- as a sanity-check, I verified (in a separate try push) that introducing an unused variable in /layout/tables will indeed trigger a build error, on Try:
> BasicTableLayoutStrategy.cpp:80:9: error: unused variable 'unused'
(that's a variable I added as a test)
 https://tbpl.mozilla.org/php/getParsedLog.php?id=8432358&tree=Try
Comment 5 Daniel Holbert [:dholbert] (largely AFK until June 28) 2012-01-09 17:45:49 PST
(I'd already verified that locally, per comment 1, but good to see that it works on Try, too. :))
Comment 6 Daniel Holbert [:dholbert] (largely AFK until June 28) 2012-01-09 20:35:32 PST
I filed 716787 on the mac issue brought up in comment 2 - comment 3.
Comment 7 Daniel Holbert [:dholbert] (largely AFK until June 28) 2012-01-10 12:41:06 PST
Comment on attachment 587199 [details] [diff] [review]
patch v1

This can't land until bug 716787 is sorted out, but in the meantime, might as well request review.  (It's a one-liner.)

Try push in comment 1 shows that it builds successfully, aside from the OS X redness due to bug 716787.  Try push in comment 4 shows that it successfully catches newly-introduced build warnings.
Comment 8 Daniel Holbert [:dholbert] (largely AFK until June 28) 2012-01-12 17:42:07 PST
(bug 716787 is now fixed, so this is ready to land as soon as it gets r+.)
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-01-19 14:37:17 PST
Comment on attachment 587199 [details] [diff] [review]
patch v1

r=dbaron
Comment 10 Daniel Holbert [:dholbert] (largely AFK until June 28) 2012-01-19 21:34:58 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d0f4e08e31e
Comment 11 Ed Morley [:emorley] 2012-01-21 07:10:25 PST
https://hg.mozilla.org/mozilla-central/rev/5d0f4e08e31e

Note You need to log in before you can comment on or make changes to this bug.