Non-unified build fails when using --enable-warnings-as-errors

RESOLVED FIXED in mozilla33

Status

()

Core
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: cpeterson, Assigned: bjacob)

Tracking

(Blocks: 1 bug)

unspecified
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox32 wontfix, firefox33 affected)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
When building with these mozconfig settings using clang on OS X:

ac_add_options --disable-unified-compilation
ac_add_options --enable-warnings-as-errors
ac_add_options --enable-sm-fail-on-warnings

I hit the following warnings-as-error build errors:

gfx/2d/FilterNodeSoftware.cpp:2216:1: error: unused function 'ColorAtPoint' [-Werror,-Wunused-function]
gfx/layers/composite/FPSCounter.cpp:368:20: error: unused variable 'FontStride' [-Werror,-Wunused-const-variable]
gfx/layers/opengl/CompositorOGL.cpp:69:23: error: unused function 'ns2gfxSize' [-Werror,-Wunused-function]
content/media/MP3FrameParser.cpp:309:23: error: unused variable 'SAMPLES_PER_FRAME' [-Werror,-Wunused-const-variable]
layout/generic/nsBlockFrame.cpp:58:23: error: unused variable 'kCircleCharacter' [-Werror,-Wunused-const-variable]
layout/generic/nsBlockFrame.cpp:59:23: error: unused variable 'kSquareCharacter' [-Werror,-Wunused-const-variable]
extensions/spellcheck/src/mozPersonalDictionary.cpp:24:11: warning: unused variable 'kMaxWordLen' [-Wunused-const-variable]
extensions/universalchardet/src/xpcom/nsUdetXPCOMWrapper.cpp:18:22: warning: unused variable 'kUniversalDetectorCID' [-Wunused-const-variable]
extensions/universalchardet/src/xpcom/nsUdetXPCOMWrapper.cpp:19:22: warning: unused variable 'kUniversalStringDetectorCID' [-Wunused-const-variable]
toolkit/components/places/nsNavHistory.cpp:105:22: warning: unused variable 'USECS_PER_DAY' [-Wunused-const-variable]
toolkit/xre/nsSigHandlers.cpp:44:18: warning: unused variable 'kClientChannelFd' [-Wunused-const-variable]
content/base/src/ShadowRoot.cpp:57:1: error: unused variable 'kDOMClassInfo_ShadowRoot_interfaces' [-Werror,-Wunused-const-variable]
layout/generic/nsSelection.cpp:3151:1: error: unused variable 'kDOMClassInfo_Selection_interfaces' [-Werror,-Wunused-const-variable]
(Assignee)

Comment 2

4 years ago
Ah, thanks for filing a bug. I have some more fixes coming up.
(Assignee)

Comment 3

4 years ago
Created attachment 8444490 [details] [diff] [review]
fix-non-unified

I'll land that if it's green on try (some platform-specific #ifdef'd stuff that I could have gotten wrong)

https://tbpl.mozilla.org/?tree=Try&rev=581374eda010
(Assignee)

Comment 4

4 years ago
Windows actually uses those -> need another try:

https://tbpl.mozilla.org/?tree=Try&rev=c90273fb0d75
># HG changeset patch
># Parent 97067b95835d08323de4e383bcde296ce64300f9
>Bug 1028420 - Non-unified build fails when using --enable-warnings-as-errors - bustage fix, no review

IMHO, this patch is stretching the definition of "bustage fix, no review"...

I don't think we should bypass review on patches, simply because they address a build failure with a special local compiler-version combined with an special mozconfig.   That is a large class of bugs (including e.g. all future bugs w/ build warnings introduced in new compiler versions, that trigger a Werror build failure w/ those compilers).

Having said that, removing unused code is generally uncontroversial, and I trust your skills/judgement, so I'm not especially concerned in this specific case.  Just commenting on the "bustage fix, no review" labeling.

(In reply to Benoit Jacob [:bjacob] from comment #4)
> Windows actually uses those -> need another try:

I assume this is referring to the static variables at the end -- kSysMemoryParameter, kTotalVirtualMemoryParameter, etc.  Are you sure Windows uses these?  (If so, where?)

A MXR search seems to indicate that they're unused, e.g.
 http://mxr.mozilla.org/mozilla-central/ident?i=kSysMemoryParameter
...and the patch that added them did include some usages (which no longer exist):
https://hg.mozilla.org/integration/mozilla-inbound/diff/b84f528e2e10/toolkit/crashreporter/nsExceptionHandler.cpp
...and the windows failures in your Try run look unrelated to those variables ("ImportError: No module named _curses"). Seems like that was just a sporadic infra issue (or maybe an issue that affected the specific trunk revision your Try run was based on). Though I may be missing something.
Flags: needinfo?(bjacob)
(Assignee)

Comment 7

4 years ago
(In reply to Daniel Holbert [:dholbert] from comment #6)
> ># HG changeset patch
> ># Parent 97067b95835d08323de4e383bcde296ce64300f9
> >Bug 1028420 - Non-unified build fails when using --enable-warnings-as-errors - bustage fix, no review
> 
> IMHO, this patch is stretching the definition of "bustage fix, no review"...

OK, point taken, I think you're right, I'll ask for review next time I want to land a similar patch.

> (In reply to Benoit Jacob [:bjacob] from comment #4)
> > Windows actually uses those -> need another try:
> 
> I assume this is referring to the static variables at the end --
> kSysMemoryParameter, kTotalVirtualMemoryParameter, etc.  Are you sure
> Windows uses these?  (If so, where?)

Those are actually used in a macro that generates identifiers with ##, which is why I initially missed those too and got windows bustage on my first try push above.

Here:
http://hg.mozilla.org/integration/mozilla-inbound/file/dd07ffb3af82/toolkit/crashreporter/nsExceptionHandler.cpp#l695
Flags: needinfo?(bjacob)
(Assignee)

Comment 8

4 years ago
And by the way, DXR should have no issue with ## (IIUC) but here this is #ifdef XP_WIN32 so this is out of reach of DXR too.
(In reply to Benoit Jacob [:bjacob] from comment #7)
> Those are actually used in a macro that generates identifiers with ##, which
> is why I initially missed those too and got windows bustage on my first try
> push above.
> 
> Here:
> http://hg.mozilla.org/integration/mozilla-inbound/file/dd07ffb3af82/toolkit/
> crashreporter/nsExceptionHandler.cpp#l695

Ah, sneaky. Thanks for the pointer.
(In reply to Daniel Holbert [:dholbert] from comment #6)
> ...and the windows failures in your Try run look unrelated to those
> variables ("ImportError: No module named _curses").

(d'oh, I was looking at the wrong Try run here -- I clicked on the link in comment 4 instead of comment 3. If I'd been looking at the right one, I would've figured this out on my own. :))
https://hg.mozilla.org/mozilla-central/rev/611283da02bf
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
(Reporter)

Updated

4 years ago
Depends on: 1031165
(Reporter)

Updated

4 years ago
Depends on: 1031166
(Reporter)

Updated

4 years ago
Depends on: 1031167
You need to log in before you can comment on or make changes to this bug.