Fix clang-13 unused variable messages. #1334
Conversation
| @@ -3641,13 +3641,11 @@ bcf_hdr_t *bcf_hdr_merge(bcf_hdr_t *dst, const bcf_hdr_t *src) | |||
| { | |||
| hts_log_warning("Trying to combine \"%s\" tag definitions of different lengths", | |||
| src->hrec[i]->vals[0]); | |||
| ret |= 1; | |||
There was a problem hiding this comment.
I get the feeling that there was an intention to do something else if this problem (or the one below) was detected. While it has been ignored for now, I think it would be good to check while our attention has been drawn to this bit of code. Maybe @pd3 could let us know how catastrophic the problems being warned about might be?
There was a problem hiding this comment.
They are all like that. Things that did something, or were meant to do something, and have been left behind. If the people who wrote it want an alternative fix then they are free to overwrite my changes.
There was a problem hiding this comment.
In this case I don't think it ever did anything. But it would be good to at least raise an issue if we think it really should be doing something, so we remember to fix it.
There was a problem hiding this comment.
It may be a problem but does not have to be. Probably best if the function was able to set an error code which could be optionally checked by the caller?
There was a problem hiding this comment.
Unfortunately the only way the function can report errors at the moment is to return NULL. While it already does for other detected problems, it never has for these. As in the case of these particular warnings you'll always have supplied a dst you could recover from a NULL return, but it would be a bit of a faff.
Presumably if you get these warnings, one of the dst headers has the wrong type. What would be the consequence of trying to use it, and is it bad enough to merit returning an error from this function?
There was a problem hiding this comment.
Sorry for not being clear, I meant if maybe errno could be (mis)used for this.
The consequence depends on the user application. The current uses in BCFtools are okay with it, another check can be performed at the data record level.
Probably best and easiest would be to provide a new function, in case it is ever needed. For now I am completely okay with leaving it as is.
There was a problem hiding this comment.
You'd have to return NULL as well as setting errno, otherwise the caller wouldn't know to check the latter (following the convention that errno is left unchanged if everything worked).
If you can check this on data records then I guess it's OK to ignore for now.
| @@ -3095,7 +3095,6 @@ cram_codec *cram_huffman_encode_init(cram_stats *st, | |||
| vals[nvals] = i; | |||
| freqs[nvals] = st->freqs[i]; | |||
| assert(st->freqs[i] > 0); | |||
| ntot += freqs[nvals]; | |||
There was a problem hiding this comment.
And it would be good if @jkbonfield can confirm that this wasn't being ignored by mistake.
There was a problem hiding this comment.
The original code had this after the main loop:
assert(ntot == st->nsamp);
It looks like it was basically debugging code during development that was culled incompletely. We could of course add a check back and handle the error in a more graceful way, but it's been working this long just fine so it seems rather pointless.
Furthermore, the entire huffman encode-side is now basically defunct, along with most of the other CORE bit-shoveling techniques. We write to "external" blocks and use rANS instead as this is fast and decouples the streams from one another permitting selective decoding. Maybe one day if we ever release CRAM 4 we can reenable the bit encoders, directing their output somewhere other than CORE block, but it would break CRAM 3 for now.
The decoder is still needed as we may find crams that use them, but for encoding the only bit of huffman we (ab)use is as a constant value encoder by generating a huffman tree with 1 symbol having a code length of 0 bits. (Plus we don't actually use the general purpose huffman decoder then as it has a custom constant-value handler.)
There was a problem hiding this comment.
Thanks. Good to be sure it wasn't supposed to be there.
|
Merged so our tests will now work. Will follow up tomorrow to check if anything needs to be done with |
The new version of clang is better at finding unused variables. This PR removes those variables.