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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 20 May 2015, "Gore, Tim" <tim.gore@intel.com> wrote:
>> -----Original Message-----
>> From: Lespiau, Damien
>> Sent: Wednesday, May 20, 2015 10:48 AM
>> To: Morton, Derek J
>> Cc: Daniel Vetter; Gore, Tim; intel-gfx@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@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.

Like both Chris and I said in reply to different versions of the patch,
having to use an explicit cast because of the signed->unsigned change is
bad too. Worse, I think.

BR,
Jani.


>  Tim
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx