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