Closed Bug 967871 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Layout, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: alex_y_xu, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → dholbert
Hardware: x86_64 → All
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.
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.)
Attached patch fix v2Splinter Review
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.
Attachment #8370428 - Flags: review?(dholbert)
Flags: needinfo?(alex_y_xu)
This fixes the warning in question, but I have not finished the build due to other warnings causing errors on 3.4.
(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).
(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 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.]
Attachment #8370428 - Flags: review?(dholbert) → feedback?(matspal)
(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 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?)
Attachment #8370415 - Flags: review?(matspal) → review+
Attachment #8370428 - Flags: feedback?(matspal)
(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. :))
I completely missed that... OK, no problem then.
https://hg.mozilla.org/mozilla-central/rev/ad19b44cab09
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.