Closed Bug 1154106 Opened 4 years ago Closed 4 years ago

6 -Wunused-const-variable compiler warnings in shlibsign.c: for h, seed, counter, h2, seed2, counter2

Categories

(NSS :: Tools, defect)

x86_64
Linux
defect
Not set

Tracking

(firefox40 affected)

RESOLVED FIXED
Tracking Status
firefox40 --- affected

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

When compiling mozilla-central with clang 3.7, I get 6 "-Wunused-const-variable" build warnings in NSS's file "shlibsign.c":
{
shlibsign.c:198:28: warning: unused variable 'h' [-Wunused-const-variable]
static const unsigned char h[] = {
                           ^
shlibsign.c:216:28: warning: unused variable 'seed' [-Wunused-const-variable]
static const unsigned char seed[] = { 0x00,
                           ^
shlibsign.c:234:27: warning: unused variable 'counter' [-Wunused-const-variable]
static const unsigned int counter=1496;
                          ^
shlibsign.c:310:28: warning: unused variable 'h2' [-Wunused-const-variable]
static const unsigned char h2[] = {
                           ^
shlibsign.c:344:28: warning: unused variable 'seed2' [-Wunused-const-variable]
static const unsigned char seed2[] = { 0x00,
                           ^
shlibsign.c:378:27: warning: unused variable 'counter2' [-Wunused-const-variable]
static const unsigned int counter2=210;
                          ^
}

HISTORY OF WHY THESE VARIABLES ARE UNUSED:
The first two -- "h" and "seed" -- had their last usages removed here, for bug 347037:
 https://hg.mozilla.org/projects/nss/rev/4b1454c98054#l3.238

The third -- "counter" -- was added, without any usages, in that same changeset for bug 347037:
 https://hg.mozilla.org/projects/nss/rev/4b1454c98054#l3.223
(I'm not clear on whether it was supposed to be used. It looks like its numeric value -- 1496 -- was being refactored out of some no-longer-needed code.)


The fourth, fifth, and sixth -- h2, seed2, counter2 -- were added, without any usages, in this cset for bug 475578:
 https://hg.mozilla.org/projects/nss/rev/d8c41dddb35e#l2.86
(I'm not clear on whether these were supposed to be used either.)
Here's this file as it exists right now:
  https://hg.mozilla.org/projects/nss/annotate/82de44ead36f/cmd/shlibsign/shlibsign.c#l198

(It's been moved since the changesets referenced in comment 0, but the unused variables are still there.)
Summary: 6 -Wunused-const-variable warnings in shlibsign.c → 6 -Wunused-const-variable compiler warnings in shlibsign.c: for h, seed, counter, h2, seed2, counter2
Trivial patch: just drop the unused variables.  Robert, would you be an appropriate reviewer for this?

(I'm hoping you might be able to tell whether these variables should really be being used, based on my archeology in comment 0 here. I'm assuming they're removable, but if not, we clearly need a more substantial bug to use them as they're intended to be used.)
Attachment #8591998 - Flags: review?(rrelyea)
I verified that I can build current NSS, locally, with this patch applied.
Component: Libraries → Tools
Comment on attachment 8591998 [details] [diff] [review]
fix v1: drop the unused variables

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

These are the verify functions values. We probably should keep them around as comments so that someone could verify that our PQG parameters were generated properly, but we don't need them in the code.
Attachment #8591998 - Flags: review?(rrelyea) → review+
(In reply to Robert Relyea from comment #5)
> These are the verify functions values. We probably should keep them around
> as comments so that someone could verify that our PQG parameters were
> generated properly, but we don't need them in the code.

Happy to restore them, commented-out, if you like. Does this apply to all of the variables in this patch? (The ones which were never used, as well as the ones which were once used but aren't anymore?)
(ni=robert for comment 6. See end of comment 0 for the breakdown of each variable's history here.)
Flags: needinfo?(rrelyea)
Here's an updated patch, which is what I think you were going for (commenting out all the unused variables, with an explanatory comment).
Assignee: nobody → dholbert
Attachment #8591998 - Attachment is obsolete: true
Status: NEW → ASSIGNED
[er, previous fix introduced an extra blank line; fixed that here.]
Attachment #8633142 - Attachment is obsolete: true
Attachment #8633143 - Flags: review?(rrelyea)
Comment on attachment 8633143 [details] [diff] [review]
fix v2a (comment out the unused variables)

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

Excellent, thanks.
Attachment #8633143 - Flags: review?(rrelyea) → review+
Flags: needinfo?(rrelyea)
Keywords: checkin-needed
https://hg.mozilla.org/projects/nss/rev/198cc75bc8bd
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.20
Keywords: checkin-needed
Comment on attachment 8633143 [details] [diff] [review]
fix v2a (comment out the unused variables)

For future reference: it is usually better to comment
out a large block of code using #if 0 and #endif.
Target Milestone: 3.20 → 3.21
(In reply to Wan-Teh Chang from comment #12)
> For future reference: it is usually better to comment
> out a large block of code using #if 0 and #endif.

FWIW, I considered doing that when I wrote the patch, but I opted to go for a /*...*/ comment instead, because:
 (1) I recall some sort of Mozilla coding-style guideline that "dead code" (including "#if 0" code) is semi-prohibited.  (Though I can't actually recall where that guideline is documented; I can't seem to find it right now, and it's not in the official coding style page.)

 (2) The disabled code here is only being preserved as documentation ("here are some parameters that were used in generating stuff here").  It's not e.g. temporarily-disabled code which we intend to re-enable later on, as far as I know.  So, putting it in a code-comment seemed more sensible (or, at least as sensible) as #ifdeffing it.

 (3) I was already including a comment to explain why this unused stuff is still there; it made sense to me to just include the variables as a contiguous part of that comment, instead of separating it out.
You need to log in before you can comment on or make changes to this bug.