[GRASS-dev] i.modis.qc

GRASS GRASS developer
1

i.modis.qc currently generates a number of warnings:

qc250b.c:14: warning: statement with no effect
qc250c.c:20: warning: statement with no effect
qc250d.c:12: warning: statement with no effect
qc250e.c:12: warning: statement with no effect
qc250f.c:12: warning: statement with no effect
qc500c.c:22: warning: statement with no effect
qc500d.c:12: warning: statement with no effect
qc500e.c:12: warning: statement with no effect

These all relate to code such as:

    qctemp >> 2; /*bits [2-3] become [0-1] */

Is this supposed to be:

    qctemp >>= 2;
or:
    pixel >>= 2;

instead?

--
Glynn Clements <glynn@gclements.plus.com>

2

Hello Glynn,

It should be standard bit shift ">>", so as to push the binary bits of
interest at the right side to be used for classification.
I do not get those warnings, and it processes as expected. Anything
special about your compilation/compiler maybe?

Yann

2008/10/24 Glynn Clements <glynn@gclements.plus.com>:

i.modis.qc currently generates a number of warnings:

qc250b.c:14: warning: statement with no effect
qc250c.c:20: warning: statement with no effect
qc250d.c:12: warning: statement with no effect
qc250e.c:12: warning: statement with no effect
qc250f.c:12: warning: statement with no effect
qc500c.c:22: warning: statement with no effect
qc500d.c:12: warning: statement with no effect
qc500e.c:12: warning: statement with no effect

These all relate to code such as:

   qctemp >> 2; /*bits [2-3] become [0-1] */

Is this supposed to be:

   qctemp >>= 2;
or:
   pixel >>= 2;

instead?

--
Glynn Clements <glynn@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/

3

Hello again Glynn,

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

Thanks,
Yann

2008/10/24 Yann Chemin <yann.chemin@gmail.com>:

Hello Glynn,

It should be standard bit shift ">>", so as to push the binary bits of
interest at the right side to be used for classification.
I do not get those warnings, and it processes as expected. Anything
special about your compilation/compiler maybe?

Yann

2008/10/24 Glynn Clements <glynn@gclements.plus.com>:

i.modis.qc currently generates a number of warnings:

qc250b.c:14: warning: statement with no effect
qc250c.c:20: warning: statement with no effect
qc250d.c:12: warning: statement with no effect
qc250e.c:12: warning: statement with no effect
qc250f.c:12: warning: statement with no effect
qc500c.c:22: warning: statement with no effect
qc500d.c:12: warning: statement with no effect
qc500e.c:12: warning: statement with no effect

These all relate to code such as:

   qctemp >> 2; /*bits [2-3] become [0-1] */

Is this supposed to be:

   qctemp >>= 2;
or:
   pixel >>= 2;

instead?

--
Glynn Clements <glynn@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/

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

4

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@gclements.plus.com>

5

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@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@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/

6

Hi again Glynn,

I checked my compilation flags, they are:
CFLAGS="-ggdb -Wall -Werror-implicit-function-declaration"

and on my compiler there is not any warning coming up on compilation
of i.modis.qc.

this is what gcc -v tells:
Using built-in specs.
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Debian
4.3.2-1' --with-bugurl=file:///usr/share/doc/gcc-4.3/README.Bugs
--enable-languages=c,c++,fortran,objc,obj-c++ --prefix=/usr
--enable-shared --with-system-zlib --libexecdir=/usr/lib
--without-included-gettext --enable-threads=posix --enable-nls
--with-gxx-include-dir=/usr/include/c++/4.3 --program-suffix=-4.3
--enable-clocale=gnu --enable-libstdcxx-debug --enable-objc-gc
--enable-mpfr --enable-cld --enable-checking=release
--build=x86_64-linux-gnu --host=x86_64-linux-gnu
--target=x86_64-linux-gnu
Thread model: posix
gcc version 4.3.2 (Debian 4.3.2-1)

Yann

2008/10/26 Glynn Clements <glynn@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@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/

7

Hi Yann,
Thought I would comment too so you know it is not just Glynn who sees there is a problem here.

On Mon, 27 Oct 2008, Yann Chemin wrote:

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.

Well, it is not in doubt that the statement with the bit shift does not do anything and has no effect. So if you feel it works OK the way it is then that statement can be safely removed with no effect.

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.

Whether or not there is a warning is a moot point (apart from the fact that because we're only finding this bug because of a warning, it suggests there could be other undiscovered bugs in the code that aren't generating warnings). Anyway as Glynn said, the point is that a statement like qctemp >> 2;
does not do anything. qctemp is not modified as a result; the bit-shifted value theoretically exists in the ether for an instant, but it is not assigned to anything so it is lost again instantly. That statement can be removed from the program and the output will be exactly the same.

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.

What do you mean? Bitshift doesn't have a unique feature; it works just like any other operator like + or - or | or &.

Paul

8

Hi Paul,

Thanks for your comments.
I go back to the code and to the books then.

Yann

2008/10/27 Paul Kelly <paul-grass@stjohnspoint.co.uk>:

Hi Yann,
Thought I would comment too so you know it is not just Glynn who sees there
is a problem here.

On Mon, 27 Oct 2008, Yann Chemin wrote:

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.

Well, it is not in doubt that the statement with the bit shift does not do
anything and has no effect. So if you feel it works OK the way it is then
that statement can be safely removed with no effect.

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.

Whether or not there is a warning is a moot point (apart from the fact that
because we're only finding this bug because of a warning, it suggests there
could be other undiscovered bugs in the code that aren't generating
warnings). Anyway as Glynn said, the point is that a statement like qctemp

2;

does not do anything. qctemp is not modified as a result; the bit-shifted
value theoretically exists in the ether for an instant, but it is not
assigned to anything so it is lost again instantly. That statement can be
removed from the program and the output will be exactly the same.

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.

What do you mean? Bitshift doesn't have a unique feature; it works just like
any other operator like + or - or | or &.

Paul

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

9

My apologies,

It should indeed be pixel in the bitshift and after Glynn comment (>>=):

    pixel >>= 2; /*bits [2-3] become [0-1] */

and not:

    qctemp >> 2; /*bits [2-3] become [0-1] */

I'll see through this.
Yann

---------- Forwarded message ----------
From: Yann Chemin <yann.chemin@gmail.com>
Date: 2008/10/27
Subject: Re: [GRASS-dev] Re: i.modis.qc
To: Paul Kelly <paul-grass@stjohnspoint.co.uk>
Cc: Glynn Clements <glynn@gclements.plus.com>, GRASS developers list
<grass-dev@lists.osgeo.org>

Hi Paul,

Thanks for your comments.
I go back to the code and to the books then.

Yann

2008/10/27 Paul Kelly <paul-grass@stjohnspoint.co.uk>:

Hi Yann,
Thought I would comment too so you know it is not just Glynn who sees there
is a problem here.

On Mon, 27 Oct 2008, Yann Chemin wrote:

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.

Well, it is not in doubt that the statement with the bit shift does not do
anything and has no effect. So if you feel it works OK the way it is then
that statement can be safely removed with no effect.

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.

Whether or not there is a warning is a moot point (apart from the fact that
because we're only finding this bug because of a warning, it suggests there
could be other undiscovered bugs in the code that aren't generating
warnings). Anyway as Glynn said, the point is that a statement like qctemp

2;

does not do anything. qctemp is not modified as a result; the bit-shifted
value theoretically exists in the ether for an instant, but it is not
assigned to anything so it is lost again instantly. That statement can be
removed from the program and the output will be exactly the same.

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.

What do you mean? Bitshift doesn't have a unique feature; it works just like
any other operator like + or - or | or &.

Paul

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

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