[Qemu-devel] [PATCH] gtk: Fix compiler warnings with -Werror=sign-compare

[Qemu-devel] [PATCH] gtk: Fix compiler warnings with -Werror=sign-compare

[Stefan Weil]

With -Werror=sign-compare (not enabled by default), gcc shows these errors:

ui/gtk.c: In function ‘gtk_release_modifiers’:
ui/gtk.c:288:19: error:
 comparison between signed and unsigned integer expressions [-Werror=sign-compare]
ui/gtk.c: In function ‘gd_key_event’:
ui/gtk.c:746:19: error:
 comparison between signed and unsigned integer expressions [-Werror=sign-compare]

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 ui/gtk.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ui/gtk.c b/ui/gtk.c
index b5f4f0b..79efda1 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -273,7 +273,8 @@ static void gd_update_full_redraw(GtkDisplayState *s)
 
 static void gtk_release_modifiers(GtkDisplayState *s)
 {
-    int i, keycode;
+    unsigned i;
+    int keycode;
 
     if (!gd_on_vga(s)) {
         return;
@@ -714,7 +715,7 @@ static gboolean gd_key_event(GtkWidget *widget, GdkEventKey *key, void *opaque)
     GtkDisplayState *s = opaque;
     int gdk_keycode;
     int qemu_keycode;
-    int i;
+    unsigned i;
 
     gdk_keycode = key->hardware_keycode;
 
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH] gtk: Fix compiler warnings with -Werror=sign-compare

[Peter Maydell]

On 4 November 2013 19:51, Stefan Weil <sw@weilnetz.de> wrote:
> With -Werror=sign-compare (not enabled by default), gcc shows these errors:
>
> ui/gtk.c: In function ‘gtk_release_modifiers’:
> ui/gtk.c:288:19: error:
>  comparison between signed and unsigned integer expressions [-Werror=sign-compare]
> ui/gtk.c: In function ‘gd_key_event’:
> ui/gtk.c:746:19: error:
>  comparison between signed and unsigned integer expressions [-Werror=sign-compare]

If this warning is going to complain about entirely
safe and idiomatic code like

   int i;
   static const int some_array[] = {
       0x2a, 0x36, 0x1d, 0x9d, 0x38, 0xb8, 0xdb, 0xdd,
   };

   for (i = 0; i < ARRAY_SIZE(some_array); i++) {
       ...
   }

then I think it is more trouble than it is worth and we
should leave it disabled.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] gtk: Fix compiler warnings with -Werror=sign-compare

[Stefan Weil]

Am 04.11.2013 21:07, schrieb Peter Maydell:
> On 4 November 2013 19:51, Stefan Weil <sw@weilnetz.de> wrote:
>> With -Werror=sign-compare (not enabled by default), gcc shows these errors:
>>
>> ui/gtk.c: In function ‘gtk_release_modifiers’:
>> ui/gtk.c:288:19: error:
>>  comparison between signed and unsigned integer expressions [-Werror=sign-compare]
>> ui/gtk.c: In function ‘gd_key_event’:
>> ui/gtk.c:746:19: error:
>>  comparison between signed and unsigned integer expressions [-Werror=sign-compare]
> If this warning is going to complain about entirely
> safe and idiomatic code like
>
>    int i;
>    static const int some_array[] = {
>        0x2a, 0x36, 0x1d, 0x9d, 0x38, 0xb8, 0xdb, 0xdd,
>    };
>
>    for (i = 0; i < ARRAY_SIZE(some_array); i++) {
>        ...
>    }
>
> then I think it is more trouble than it is worth and we
> should leave it disabled.
>
> thanks
> -- PMM

Yes, we should leave it disabled for simple practical reasons: there are
too many warnings of this kind, so I don't expect they will be fixed in
the near future (I have no plan to send patches to fix them all - this
one was just in my way).

And yes, we should fix code which can be improved as easily as in the
above example. ARRAY_SIZE happens to be an unsigned size_t, so comparing
it to an unsigned value is better IMHO (I'm aware that there were
already discussions about using signed or unsigned integers).

-Wsign-compare is like other compiler warnings: it can detect some
really unwanted code, but a human reviewer won't always agree on the
results.It might be a good idea to support it somewhere in the far future.

Stefan

Re: [Qemu-devel] [PATCH] gtk: Fix compiler warnings with -Werror=sign-compare

[Laszlo Ersek]

On 11/04/13 21:07, Peter Maydell wrote:
> On 4 November 2013 19:51, Stefan Weil <sw@weilnetz.de> wrote:
>> With -Werror=sign-compare (not enabled by default), gcc shows these errors:
>>
>> ui/gtk.c: In function ‘gtk_release_modifiers’:
>> ui/gtk.c:288:19: error:
>>  comparison between signed and unsigned integer expressions [-Werror=sign-compare]
>> ui/gtk.c: In function ‘gd_key_event’:
>> ui/gtk.c:746:19: error:
>>  comparison between signed and unsigned integer expressions [-Werror=sign-compare]
> 
> If this warning is going to complain about entirely
> safe and idiomatic code like
> 
>    int i;
>    static const int some_array[] = {
>        0x2a, 0x36, 0x1d, 0x9d, 0x38, 0xb8, 0xdb, 0xdd,
>    };
> 
>    for (i = 0; i < ARRAY_SIZE(some_array); i++) {
>        ...
>    }

(Entirely safe, and completely non-idiomatic: "i" should be size_t, as
that is the type of the sizeof operator's result.)

Laszlo
/me apologizes for the raging pedantry

Re: [Qemu-devel] [PATCH] gtk: Fix compiler warnings with -Werror=sign-compare

[Peter Maydell]

On 4 November 2013 20:38, Laszlo Ersek <lersek@redhat.com> wrote:
> On 11/04/13 21:07, Peter Maydell wrote:
>> If this warning is going to complain about entirely
>> safe and idiomatic code like
>>
>>    int i;
>>    static const int some_array[] = {
>>        0x2a, 0x36, 0x1d, 0x9d, 0x38, 0xb8, 0xdb, 0xdd,
>>    };
>>
>>    for (i = 0; i < ARRAY_SIZE(some_array); i++) {
>>        ...
>>    }
>
> (Entirely safe, and completely non-idiomatic: "i" should be size_t, as
> that is the type of the sizeof operator's result.)

Making simple loop iteration variables 'unsigned' is completely
non-idiomatic. It just happens that we're calculating the upper
bound here via a macro that coincidentally produces an
unsigned result. Insisting on unsigned also runs you into
problems with loops like

   for (i = ARRAY_SIZE(foo) - 1; i >=0 ; i--) {
     something(foo[i]);
   }

(which obviously you can avoid by contorting the code,
but why do that when leaving 'i' as signed is much clearer?)

-- PMM

[Qemu-devel] C99 loop vars? [was: gtk: Fix compiler warnings with -Werror=sign-compare]

[Michael Tokarev]

05.11.2013 00:38, Laszlo Ersek wrote:
> On 11/04/13 21:07, Peter Maydell wrote:
>> On 4 November 2013 19:51, Stefan Weil <sw@weilnetz.de> wrote:
>>> With -Werror=sign-compare (not enabled by default), gcc shows these errors:
>>>
>>> ui/gtk.c: In function ‘gtk_release_modifiers’:
>>> ui/gtk.c:288:19: error:
>>>   comparison between signed and unsigned integer expressions [-Werror=sign-compare]
>>> ui/gtk.c: In function ‘gd_key_event’:
>>> ui/gtk.c:746:19: error:
>>>   comparison between signed and unsigned integer expressions [-Werror=sign-compare]
>>
>> If this warning is going to complain about entirely
>> safe and idiomatic code like
>>
>>     int i;
>>     static const int some_array[] = {
>>         0x2a, 0x36, 0x1d, 0x9d, 0x38, 0xb8, 0xdb, 0xdd,
>>     };
>>
>>     for (i = 0; i < ARRAY_SIZE(some_array); i++) {
>>         ...
>>     }
>
> (Entirely safe, and completely non-idiomatic: "i" should be size_t, as
> that is the type of the sizeof operator's result.)

Maybe in some places we should switch to C99 which allows to declare
a loop variable inside the loop header, like this:

   for(int i = 0; i < ARRAY_SIZE(..); i++) {
   }

?

This is much better than a per-unit (function-level) declaration because
by changing type in one place we don't change it for other places which
might become wrong in the result...

But this requires compiling whole thing with gcc -std=c99, which might
bit problematic.

But as it is, the original patch from Stefan, -- I don't think it's a
good idea to apply it.  The code is obviously correct, ARRAY_SIZE is
a fixed value, the compiler is able to determine this and shut pretty
much up ;)

Thanks,

/mjt