SSLServerCertVerification.cpp:284:21: warning: private field 'mFdForLogging' is not used [Wunused-private-field]

RESOLVED FIXED in mozilla30

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla30
All
Mac OS X
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Clang warning that we're currently hitting on opt Mac TBPL builds (which use clang):
{ 
security/manager/ssl/src/SSLServerCertVerification.cpp:284:21: warning: private field 'mFdForLogging' is not used [Wunused-private-field]
}

This mFdForLogging variable is only used in a PR_LOG statement, which is #defined away in opt builds, leaving the variable unused.

We could play some tricks by #ifdeffing around the variable, but given that there's another variable of the same name in the same file which *is* used in opt builds (by passing it as a function-arg), I think it's probably easier/saner to just pass this unused version to 'unused <<' (alongside the PR_LOG call, so that we'll notice to drop the unused << and the variable if we ever drop the PR_LOG).

Patch coming up to do that.
(Assignee)

Comment 1

5 years ago
(FWIW, here's a log from a Try run, with this directory marked as FAIL_ON_WARNINGS in a separate local patch, tripping over this build warning:
 https://tbpl.mozilla.org/php/getParsedLog.php?id=34095391&tree=Try)
(Assignee)

Comment 2

5 years ago
Created attachment 8370912 [details] [diff] [review]
fix v1
Attachment #8370912 - Flags: review?(brian)
(Assignee)

Updated

5 years ago
Depends on: 968363
Comment on attachment 8370912 [details] [diff] [review]
fix v1

Review of attachment 8370912 [details] [diff] [review]:
-----------------------------------------------------------------

Honestly, I'm not even sure mFdForLogging is useful. I say we just remove it.
(Assignee)

Updated

5 years ago
Blocks: 968363
No longer depends on: 968363
(Assignee)

Comment 4

5 years ago
Just this CertErrorRunnable::mFdForLogging, or the other one too? (SSLServerCertVerificationJob::mFdForLogging)
mFdForLogging is useful for reading logs to see which certificate verification is tied to which connection. Please don't remove it.

Can't we use DebugOnly<T> for this?
(Assignee)

Comment 6

5 years ago
(In reply to Brian Smith (:briansmith, :bsmith; NEEDINFO? for response) from comment #5)
> Can't we use DebugOnly<T> for this?

No. DebugOnly is #ifdef DEBUG, whereas the usages of this variable are #ifdef PR_LOGGING.

If you'd like, I can mark this variable as #ifdef PR_LOGGING though.

(I had an earlier patch to do that, and then I tried applying it to SSLServerCertVerificationJob's variable as well, and got a build error, because SSLServerCertVerificationJob passes its mFdForLogging as a function-arg in at least one spot. So rather than having two classes one of which has an #ifdeffed variable and the other has a non-#ifdeffed variable, I opted for this unused << solution.)
(Assignee)

Comment 7

5 years ago
(TBPL would be happy with a DebugOnly<> solution, because our debug builds have PR_LOGGING defined and our opt builds do not. That'd just be luck, though, and we'd run into trouble if anyone tried to turn on logging in an opt build.)
Comment on attachment 8370912 [details] [diff] [review]
fix v1

Review of attachment 8370912 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the explanation.
Attachment #8370912 - Flags: review?(brian) → review+
(Assignee)

Comment 9

5 years ago
Thanks for the review!

I noticed mercurial wanted to touch a few additional lines when I was preparing to push this, and it turned out it was just auto-fixing DOS newlines in this file. (might be due to something in my .hgrc, not sure)  Rather than letting it auto-lump those fixes into this cset, I did a quick "fromdos" run to fix those lines on their own:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/4dc3ca14787f
and then landed this bug's actual patch:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/214c6b4f03c7
(Assignee)

Updated

5 years ago
Flags: in-testsuite-
Thanks for the newline fixes. Unfortunately I use MS Visual Studio as an editor, which works great except it doesn't have a "Unix Newlines By Default" option. I will figure out a solution to reduce the chances of me adding more DOS newlines into my patches.
(Assignee)

Comment 11

5 years ago
(http://stackoverflow.com/questions/3802406/configure-visual-studio-with-unix-end-of-lines has a suggestion for that, though maybe it's more subtle than that.)

Also: I could've sworn this file already had an #include for "unused.h", but apparently it doesn't (and hence comment 9 busted inbound).  Oops. Pushed a followup to add the #include and fix the bustage:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/f0eb0b8c20d0
(Assignee)

Comment 12

5 years ago
(In reply to Daniel Holbert [:dholbert] from comment #11)
> Also: I could've sworn this file already had an #include for "unused.h", but
> apparently it doesn't (and hence comment 9 busted inbound).  Oops.

Aha! I'm not crazy. I *did* have an #include for "unused.h" **in my local copy of this file**, because I initially stacked this on top of the first version of my fix for bug 968348 (which added this #include, to mark an unrelated local variable as unused).

The problem was that I wrote the patches in that order, but then pushed this one first. :) In any case, all's well now.
https://hg.mozilla.org/mozilla-central/rev/214c6b4f03c7
https://hg.mozilla.org/mozilla-central/rev/f0eb0b8c20d0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.