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

Status: RESOLVED FIXED

(NSS :: Tools, defect)

Description

[Daniel Holbert [:dholbert]]

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.)

Comment 1

[Daniel Holbert [:dholbert]]

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.)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1: drop the unused variables

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.)

Comment 3

[Daniel Holbert [:dholbert]]

I verified that I can build current NSS, locally, with this patch applied.

Comment 4

[Daniel Holbert [:dholbert]]

(by which I mean up-to-date https://hg.mozilla.org/projects/nss/ )

Comment 5

[Robert Relyea]

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.

Comment 6

[Daniel Holbert [:dholbert]]

(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?)

Comment 7

[Daniel Holbert [:dholbert]]

(ni=robert for comment 6. See end of comment 0 for the breakdown of each variable's history here.)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: fix v2: comment out the unused variables

Here's an updated patch, which is what I think you were going for (commenting out all the unused variables, with an explanatory comment).

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: fix v2a (comment out the unused variables)

[er, previous fix introduced an extra blank line; fixed that here.]

Comment 10

[Robert Relyea]

Comment on attachment 8633143 [details] [diff] [review]
fix v2a (comment out the unused variables)

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

Excellent, thanks.

Comment 11

[Kai Engert [:KaiE:]]

https://hg.mozilla.org/projects/nss/rev/198cc75bc8bd

Comment 12

[Wan-Teh Chang]

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.

Comment 13

[Daniel Holbert [:dholbert]]

(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.