On Wed, 20 May 2015, "Gore, Tim" <tim.gore@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx; Wood, Thomas
>> Subject: Re: [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@xxxxxxxxxxxxxxxxxxxxx
>> > >> > 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
