Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix clang-13 unused variable messages. #1334

Merged
merged 1 commit into from
Sep 27, 2021
Merged

Fix clang-13 unused variable messages. #1334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions bgzf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1467,7 +1467,7 @@ static void *bgzf_mt_writer(void *vp) {
int bgzf_mt_read_block(BGZF *fp, bgzf_job *j)
{
uint8_t header[BLOCK_HEADER_LENGTH], *compressed_block;
int count, size = 0, block_length, remaining;
int count, block_length, remaining;

// NOTE: Guaranteed to be compressed as we block multi-threading in
// uncompressed mode. However it may be gzip compression instead
Expand Down Expand Up @@ -1496,7 +1496,6 @@ int bgzf_mt_read_block(BGZF *fp, bgzf_job *j)
if (count != sizeof(header)) // no data read
return -1;

size = count;
block_length = unpackInt16((uint8_t*)&header[16]) + 1; // +1 because when writing this number, we used "-1"
if (block_length < BLOCK_HEADER_LENGTH) {
j->errcode |= BGZF_ERR_HEADER;
Expand All @@ -1510,7 +1509,6 @@ int bgzf_mt_read_block(BGZF *fp, bgzf_job *j)
j->errcode |= BGZF_ERR_IO;
return -1;
}
size += count;
j->comp_len = block_length;
j->uncomp_len = BGZF_MAX_BLOCK_SIZE;
j->block_address = block_address;
Expand Down
4 changes: 1 addition & 3 deletions cram/cram_codecs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3069,7 +3069,7 @@ cram_codec *cram_huffman_encode_init(cram_stats *st,
int version, varint_vec *vv) {
int *vals = NULL, *freqs = NULL, *lens = NULL, code, len;
int *new_vals, *new_freqs;
int i, ntot = 0, max_val = 0, min_val = INT_MAX, k;
int i, max_val = 0, min_val = INT_MAX, k;
size_t nvals, vals_alloc = 0;
cram_codec *c;
cram_huffman_code *codes;
Expand All @@ -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];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And it would be good if @jkbonfield can confirm that this wasn't being ignored by mistake.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Good to be sure it wasn't supposed to be there.

if (max_val < i) max_val = i;
if (min_val > i) min_val = i;
nvals++;
Expand All @@ -3118,7 +3117,6 @@ cram_codec *cram_huffman_encode_init(cram_stats *st,
vals[nvals]= kh_key(st->h, k);
freqs[nvals] = kh_val(st->h, k);
assert(freqs[nvals] > 0);
ntot += freqs[nvals];
if (max_val < i) max_val = i;
if (min_val > i) min_val = i;
nvals++;
Expand Down
4 changes: 1 addition & 3 deletions vcf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3584,7 +3584,7 @@ bcf_hdr_t *bcf_hdr_merge(bcf_hdr_t *dst, const bcf_hdr_t *src)
return dst;
}

int i, ndst_ori = dst->nhrec, need_sync = 0, ret = 0, res;
int i, ndst_ori = dst->nhrec, need_sync = 0, res;
for (i=0; i<src->nhrec; i++)
{
if ( src->hrec[i]->type==BCF_HL_GEN && src->hrec[i]->value )
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
if ( (kh_val(d_src,k_src).info[rec->type]>>4 & 0xf) != (kh_val(d_dst,k_dst).info[rec->type]>>4 & 0xf) )
{
hts_log_warning("Trying to combine \"%s\" tag definitions of different types",
src->hrec[i]->vals[0]);
ret |= 1;
}
}
}
Expand Down