ash: attempt to correct some ash conversion warnings
Cristian Ionescu-Idbohrn
cristian.ionescu-idbohrn at axis.com
Wed Oct 14 13:40:40 UTC 2009
On Sun, 11 Oct 2009, Denys Vlasenko wrote:
> And the total is staggering:
>
> # grep warning make.log | wc -l
> 3153
Yes. And there's a risk it gets worse. But as long '-Wconversion' is
commented out, we don't see the warnings which then seem not to exist.
> Seems like significant part of them are false positives.
> for example:
>
> for (i = 0; i < nblock; i++) {
> j = eclass8[i];
> k = ftab[j] - 1;
> ftab[j] = k;
> fmap[k] = i; <-------
> }
>
> archival/bz/blocksort.c:260: warning: conversion to 'uint32_t' from 'int32_t' may change the sign of the result
>
> What to do? fmap[k] = (uint32_t)i; ?
Something similar is done a few lines down:
263: nBhtab = 2 + ((uint32_t)nblock / 32); /* bbox: unsigned div is easier */
Does 'i' _have to_ be int32_t? Looks like it's because nblock is
int32_t. Theoretically, nblock could be negative and there's nothing
in the function that validates that.
Anyway, 'i' iterates from 0 to something '< nblock', and it can't ever
get negative in that loop. After validation (nblock >= 0), I would do
it like this:
uint32_t i;
> Or this:
>
> k = fmap[i] - H;
> if (k < 0)
> k += nblock;
> eclass[k] = j;
>
> fmap[i] and H are uint32_t, k is int32_t, and it CAN be negative.
Right. And, theoretically, k may be < 0 even after:
if (k < 0)
k += nblock;
and:
eclass[k] = j;
will misbehave. More code is needed to get it right.
> If you can deal with some if them by changing signedness of the
> variables, that is ok.
That is obvious.
> Adding casts, and widening variables is less desirable.
Yes. But how do you do it. Just casting is not smart enough.
> Prototype's return type is for compiler to know in which register(s)
> it can find the result (i.e. how wide is it).
> There is no hard rule "you must not use implicit narrowing
> conversion, ever".
Neither did I see any encouraging rule saying "thy _shall_ use
implicit narrowing conversion, whenever thy can".
Is it that smart and tempting to often push your luck just a little
bit more in order to shrink the code size with a few more bytes (yes,
I know, they add up; but still...)? Isn't it so that every time you
change a variable to a shorter type there's a risk you cause even more
conversion warnings, if the change is type inconsistent?
> How do you survive the code which uses xmalloc
> or xasprintf then? It can fail, you know...
> "sky is falling, run for your lives!" ?...
Right. This stuff is only for the brave :)
What do other people think?
Noone cares?
Cheers,
--
Cristian
More information about the busybox
mailing list