967871 - clang build failure with --enable-warnings-as-errors - layout/base/nsDocumentViewer.cpp:99:19: error: unused variable 'sPrintOptionsContractID' [-Werror,-Wunused-const-variable]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Alex Xu]

62:49.19 /home/alex/gecko-dev/layout/base/nsDocumentViewer.cpp:99:19: error: unused variable 'sPrintOptionsContractID' [-Werror,-Wunused-const-variable]
62:49.19 static const char sPrintOptionsContractID[]         = "@mozilla.org/gfx/printsettings-service;1";
62:49.19                   ^
62:51.76 1 error generated.

Comment 1

[Daniel Holbert [:dholbert]]

That variable is used here:
> 2626 #ifdef NS_PRINTING
[...]
> 2629 #ifdef DEBUG
[...]
> 2636   nsCOMPtr<nsIPrintOptions> printOptions = do_GetService(sPrintOptionsContractID, &rv);
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#2636

So in non-debug builds, and/or builds with printing disabled, this variable is indeed unused.

Comment 2

[Daniel Holbert [:dholbert]]

er, never mind about "builds with printing disabled" - the variable decl is already in an #ifdef NS_PRINTING block.

We just need to wrap it in #ifdef DEBUG, so that opt builds don't see it and complain.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Comment 4

[Daniel Holbert [:dholbert]]

Alex, could you confirm that this fixes the problem for you?

(The clang version I'm currently using isn't new enough to have the Wunused-const-variable warning, plus my local builds are all debug builds & wouldn't hit this, so I can't directly test this at the moment.)

Comment 5

[Alex Xu]

Attachment: fix v2

clang version 3.4 (tags/RELEASE_34/final)
Target: x86_64-pc-linux-gnu
Thread model: posix
Selected GCC installation: /usr/lib/gcc/x86_64-pc-linux-gnu/4.8.2

My fix was to just inline it, since there's only one use.

Comment 6

[Alex Xu]

This fixes the warning in question, but I have not finished the build due to other warnings causing errors on 3.4.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Alex Xu from comment #5)
> My fix was to just inline it, since there's only one use.

This is true of all the other files that use it, too.

I left it non-inline, for consistency with those other usages, and I think I marginally prefer that.

(In reply to Alex Xu from comment #6)
> This fixes the warning in question, but I have not finished the build due to
> other warnings causing errors on 3.4.

You may be interested in the patch on bug 928808 (which fixes a build warning/error that I hit in clang 3.4 or 3.5, I can't remember which).

Comment 8

[Alex Xu]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> (In reply to Alex Xu from comment #5)
> > My fix was to just inline it, since there's only one use.
> 
> This is true of all the other files that use it, too.
> 
> I left it non-inline, for consistency with those other usages, and I think I
> marginally prefer that.
> 
> (In reply to Alex Xu from comment #6)
> > This fixes the warning in question, but I have not finished the build due to
> > other warnings causing errors on 3.4.
> 
> You may be interested in the patch on bug 928808 (which fixes a build
> warning/error that I hit in clang 3.4 or 3.5, I can't remember which).

Yes, I applied that. That fixes one, but I am also running into bug 967006 and bug 967927.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8370428 [details] [diff] [review]
fix v2

[per beginning of comment 7, I prefer my #ifdeffing patch ("fix v1"), for consistency's sake, and also since it's reasonably good coding style to have long strings defined as constants at the top of the file, to leave the actual logic relatively clean.

Rather than marking this review-, I'll punt to mats (who's already tagged for review on my earlier patch) for feedback, and he can pick the approach that he prefers.]

Comment 10

[Alex Xu]

(In reply to Daniel Holbert [:dholbert] from comment #9)
> Comment on attachment 8370428 [details] [diff] [review]
> fix v2
> 
> [per beginning of comment 7, I prefer my #ifdeffing patch ("fix v1"), for
> consistency's sake, and also since it's reasonably good coding style to have
> long strings defined as constants at the top of the file, to leave the
> actual logic relatively clean.
> 
> Rather than marking this review-, I'll punt to mats (who's already tagged
> for review on my earlier patch) for feedback, and he can pick the approach
> that he prefers.]

Sounds good. I was already going in that direction, but wasn't sure exactly how to set up the flags to make it clear.

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 8370415 [details] [diff] [review]
fix v1

I slightly prefer this version since it makes it clear that this
file use that service.  That said, shouldn't the condition be
"#if defined(NS_PRINTING) && defined(DEBUG)" to match the call site?
(otherwise it'll warn in a DEBUG build without NS_PRINTING, right?)

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #11)
> That said, shouldn't the condition be
> "#if defined(NS_PRINTING) && defined(DEBUG)" to match the call site?
> (otherwise it'll warn in a DEBUG build without NS_PRINTING, right?)

I thought so at first too, but it turns out this is already inside an #ifdef NS_PRINTING chunk.

(I probably should've made the patch have enough context to show the #ifdef -- though to prove I'm not crazy, it does have "#endif // NS_PRINTING" in the ending context. :))

Comment 13

[Mats Palmgren (inactive)]

I completely missed that... OK, no problem then.

Comment 14

[Daniel Holbert [:dholbert]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/ad19b44cab09

Comment 15

[Carsten Book [:Tomcat]]

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