[Intel-gfx] [PATCH i-g-t] libs/igt_core.c: Fix compile warnings in igt_core.c

[Gore, Tim]

> -----Original Message-----
> From: Lespiau, Damien
> Sent: Wednesday, May 20, 2015 10:48 AM
> To: Morton, Derek J
> Cc: Daniel Vetter; Gore, Tim; intel-gfx at lists.freedesktop.org; Wood, Thomas
> Subject: Re: [Intel-gfx] [PATCH i-g-t] libs/igt_core.c: Fix compile warnings in
> igt_core.c
> 
> On Wed, May 20, 2015 at 08:37:56AM +0000, Morton, Derek J wrote:
> >
> > >> > -----Original Message-----
> > >> > From: Morton, Derek J
> > >> > Sent: Tuesday, May 19, 2015 12:21 PM
> > >> > To: intel-gfx at lists.freedesktop.org
> > >> > Cc: Wood, Thomas; Gore, Tim; Morton, Derek J
> > >> > Subject: [PATCH i-g-t] libs/igt_core.c: Fix compile warnings in
> > >> > igt_core.c
> > >> >
> > >> > Fixed variables incorrectly declared as signed instead of unsigned.
> > >
> > >Which kind of compiler warning is this? Imo
> > >
> > >	for (unsigned int i = 0; i < something; i++)
> > >
> > >is just not C style, the int there is perfectly fine. We've had this
> > >problem before with Android going ridiculously overboard with
> > >>compiler warnings, and the solution back then was to remove all the
> > >>silly ones for igt. It smells like the same is appropriate for this
> > >>>one here too.
> > >
> >
> > The warning occurs because 'something' is of an unsigned type. In this
> > case the macro ARRAY_SIZE uses sizeof() which returns an unsigned
> > type. Implicit conversion between signed and unsigned types has always
> > resulted in compiler warnings in my experience. It is not something
> > android specific.
> 
> unsigned int like that is C99 and the sizeof operator is a size_t, so maybe use
> that instead of unsigned int?
> 
> >
> > >> > Fixed 'unused parameter' warning from signal handlers that were
> > >> > not using the signal parameter.
> > >
> > >Yeah unused variable because you compile out assert isn't good, it
> > >will at least break a bunch of library unit tests (the ones in
> > >>lib/tests). I guess you don't run them in your Android builds?
> > >>-Daniel
> >
> > I have no idea why the asserts are compiled out for android. Tim had
> > some suggestions which need investigating. A subject for another patch
> > I guess.  We do not run the unit tests on android because it has not
> > been possible to run them since they were moved to libs/test. The
> > android make file was never updated when they were moved.  I need to
> > look at look at writing a new unit test for the fatal_signal_handler
> > so will look at getting them to build for android at the same time.
> >
> > As the unused parameter changes are more controversial I will remove
> > them for now and update the patch to just fix the signed / unsigned
> > warnings.
> 
> FWIW, I'd used the gcc unused attribute for things like that instead of those
> void expressions.
> 
> #define __unused __attribute__((unused))
> 
> void foo(int bar __unused)
> {
>   ...
> }
> 
> --
> Damien

My penny's worth is this. Most of the time assigning between signed and unsigned
is just due to sloppy coding. We all do it for sure, and I see it everywhere. But it can
lead to problems and it is very kind of the compiler to warn you. By default GCC only
warns you in C++, but I think Google must have tweaked their GCC build, I cannot
see it turned on in our build command. Fixing these is trivial, does not introduce
extra code and reduces the noise when building for Android. And its even good
style (unlike this sentence). I agree with Damien that size_t is probably better
in this case.
 Tim