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

[Glynn Clements]

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>