[GRASS-dev] Re: i.modis.qc

[Yann Chemin]

Hello Glynn,

since I rewrote the code, and tested it, I know quite much what I do
with the bitshifts, the image input, and the classification resulting
from bitshifts.

I have not seen so far any need to >>= instead of >>, in any of the
related code I read.
My worry in this email below was that a non-Unix platform compiler
(which I don't have) would cough on it for some unknown reason to me.

As I can imagine, the compiler would find strange there is no variable
data allocation in a line of code, but I would expect it to understand
the unique feature of bitshift too.

I will check compilation with -Wall and read on bitshift compilation
warnings across platform,
you are still running on Windows platform, aren't you?

Thanks,
Yann


2008/10/26 Glynn Clements <glynn at gclements.plus.com>:
>
> Yann Chemin wrote:
>
>> maybe it is wiser indeed to change to ">>=" for platform/compiler
>> stability as it forces the variable to be updated.
>> Somehow it is not necessary on my system (Debian/Sid).
>
> If you want to modify the variable, you need to use >>=. >> just
> returns the shifted value, which is pointless in this particular case.
>
> A statement like:
>
>        x >> 1;
>
> makes no more sense than:
>
>        x + 1;
> or:
>        x * 2;
>
> There's no point evaluating an expression unless you're going to do
> something with the result, e.g. assign it to a variable, or pass it to
> a function which will make use of it.
>
> If you aren't getting warnings, that's probably because your CFLAGS
> doesn't include -Wall or similar. The expression is perfectly valid C,
> so you won't get a warning by default. -Wall enables various optional
> warnings, including warnings for code which is valid but which might
> not do what you expect (e.g. "if (x = 0) ..." is valid, but it's more
> likely to be a typo for "if (x == 0) ...").
>
> However, there is still the question of which variable should be
> updated. E.g. in qc250b():
>
>    qctemp >> 2;                /*bits [2-3] become [0-1] */
>    qctemp = pixel & 0x03;
>
> Changing this to >>=:
>
>    qctemp >>= 2;               /*bits [2-3] become [0-1] */
>    qctemp = pixel & 0x03;
>
> is equally pointless, as you would be modifying qctemp then
> immediately discarding the modified value. Both of the above versions
> are equivalent to just:
>
>    qctemp = pixel & 0x03;
>
> Which leads me to suspect that it's "pixel" which should have been
> shifted, not qctemp.
>
> If you don't understand what this code is supposed to do, please
> remove it. I don't understand what it's supposed to do either, so I
> can only notice things which are obvious mistakes, not anything which
> *could* plausibly be correct but actually isn't.
>
> --
> Glynn Clements <glynn at gclements.plus.com>
>



-- 
Yann Chemin
International Rice Research Institute
Office: http://www.irri.org/gis
Perso: http://www.freewebs.com/ychemin
YiKingDo: http://yikingdo.unblog.fr/