Re: [PATCH] ANSI C correctness + warning fixes

[otaylor fresnel labs redhat com]

Martin Baulig <martin home-of-linux org> writes:

> Hi guys,
> 
> here comes a longer patch which fixes a lot of smaller, basically
> trival things such as adding missing casts, turning text following
> #endif into comments and so on.
> 
> The whole thing compiles now with
> 
>         -ansi -pedantic-warnings -D_POSIX_SOURCE -D_GNU_SOURCE

Thanks for working at this. I'm quite uncomfortable about the
number of casts that you've introduced.

Each cast that one introduces is a bug that the compiler
can't find in the future. (That is, the casts you introduced
are usually changing signedness; but they will also prevent
the compiler catching a complete pointer type mismatch.)

The casts between function pointer types and gpointer are 
inevitable, and thus OK.

But, a cast like:

@@ -1572,7 +1572,7 @@ gtk_window_move_resize (GtkWindow *windo
   saved_last_info = info->last;
 
   gtk_widget_size_request (widget, NULL);
-  gtk_window_compute_default_size (window, &new_width, &new_height);
+  gtk_window_compute_default_size (window, (guint *) &new_width, (guint *) &new_height);
   
   if (info->last.width < 0 ||
       info->last.width != new_width ||


Would be better if you simply changed the (static internal)
function gtk_window_compute_default_size to take a gint.

Whenever I run into a place where I need to introduce a cast,
I tend to look around and see if there is any way I can avoid
introducing the cast. In fact, a lot of the places where you've
added casts were actually noticed quite a while ago and left
in as essentially FIXME's.

====
@@ -220,6 +220,7 @@ gdk_x11_convert_grab_status (gint status
     }
 
   g_assert_not_reached();
+  return 0;
 }
====

Maybe we should change g_assert_not_reached() to actually be
detectable by the compiler as not returning. The easiest
way to do this would be to introduce a g_log_noreturn() is
identical to g_log() but has __attribute__ ((noreturn)); on
it.

=============
Index: gdk-pixbuf/io-jpeg.c
===================================================================
RCS file: /cvs/gnome/gtk+/gdk-pixbuf/io-jpeg.c,v
retrieving revision 1.31
diff -u -u -p -r1.31 io-jpeg.c
--- gdk-pixbuf/io-jpeg.c	2000/07/26 11:32:40	1.31
+++ gdk-pixbuf/io-jpeg.c	2000/09/29 15:14:26
@@ -50,7 +50,7 @@
 #include "gdk-pixbuf-private.h"
 #include "gdk-pixbuf-io.h"
 
-#ifndef HAVE_SIGSETJMP
+#if !defined (HAVE_SIGSETJMP) && !defined (sigsetjmp)
 #define sigjmp_buf jmp_buf
 #define sigsetjmp(jb, x) setjmp(jb)
 #define siglongjmp longjmp
==============

This looks wrong to me; we should change the configure
check to catch sigsetjmp defined as a macro if this is
needed. (Actually, the better alternative is probably
to check in configure for siglongjmp instead of sigsetjmp.
sigsetjmp is allowed to be a macro, but siglongjmp must
be an actual function.)

============
--- gdk-pixbuf/pixops/timescale.c	2000/07/22 23:50:19	1.5
+++ gdk-pixbuf/pixops/timescale.c	2000/09/29 15:14:27
@@ -181,8 +181,8 @@ int main (int argc, char **argv)
 		start_timing ();
 		for (i = 0; i < ITERS; i++)
 		  {
-		    pixops_scale (dest_buf, 0, 0, dest_width, dest_height, dest_rowstride, dest_channels, dest_has_alpha,
-				  src_buf, src_width, src_height, src_rowstride, src_channels, src_has_alpha,
+		    pixops_scale ((guchar *)dest_buf, 0, 0, dest_width, dest_height, dest_rowstride, dest_channels, dest_has_alpha,
+				  (guchar *)src_buf, src_width, src_height, src_rowstride, src_channels, src_has_alpha,
 				  (double)dest_width / src_width, (double)dest_height / src_height,
 				  filter_level);
=============

Just another example of where introducing casts is the wrong thing
to do. You should instead change dest_buf and src_buf to be guchar.

===========
Index: gtk/gtkclipboard.c
===================================================================
RCS file: /cvs/gnome/gtk+/gtk/gtkclipboard.c,v
retrieving revision 1.1
diff -u -u -p -r1.1 gtkclipboard.c
--- gtk/gtkclipboard.c	2000/09/14 16:41:19	1.1
+++ gtk/gtkclipboard.c	2000/09/29 15:14:31
@@ -629,7 +629,7 @@ request_text_received_func (GtkClipboard
   RequestTextInfo *info = data;
   gchar *result = NULL;
 
-  result = gtk_selection_data_get_text (selection_data);
+  result = (gchar *) gtk_selection_data_get_text (selection_data);
 
   if (!result)
     {
===========

Please change gtk_selection_data_get/set_text to take gchar *.

===========
+  { GDK_ACTION_ASK,   (const char *) action_ask_bits,  (const char *) action_ask_mask_bits,  NULL },
+  { GDK_ACTION_COPY,  (const char *) action_copy_bits, (const char *) action_copy_mask_bits, NULL },
+  { GDK_ACTION_MOVE,  (const char *) action_move_bits, (const char *) action_move_mask_bits, NULL },
+  { GDK_ACTION_LINK,  (const char *) action_link_bits, (const char *) action_link_mask_bits, NULL },
+  { 0              ,  (const char *) action_none_bits, (const char *) action_none_mask_bits, NULL },
============

Please change the type of these pointers to guchar * and move the
casts to when gdk_bitmap_create_from_data() is called. (Since I'd
consider the prototype of that function the problem.)

=============
diff -u -u -p -r1.69 gtkhandlebox.c
--- gtk/gtkhandlebox.c	2000/07/26 11:32:44	1.69
+++ gtk/gtkhandlebox.c	2000/09/29 15:14:45
@@ -753,7 +753,7 @@ gtk_handle_box_paint (GtkWidget      *wi
   bin = GTK_BIN (widget);
   hb = GTK_HANDLE_BOX (widget);
 
-  gdk_window_get_size (hb->bin_window, &width, &height);
+  gdk_window_get_size (hb->bin_window, (gint *) &width, (gint *) &height);
   
   if (!event)
    gtk_paint_box(widget->style,
==============

Change types of variables, remove casts.

==============
       gtk_debug_flags = g_parse_debug_string (env_string,
-					      gtk_debug_keys,
+					      (GDebugKey *) gtk_debug_keys,
 					      gtk_ndebug_keys);
==============

I think you should be able to change g_parse_debug_string to take
'const GDebugKey *' without any backwards compatibility breakage.

===========
-  klass->add_accelerator = (void*) gtk_accel_group_handle_add;
-  klass->remove_accelerator = (void*) gtk_accel_group_handle_remove;
+  klass->add_accelerator = gtk_widget_real_add_accelerator;
+  klass->remove_accelerator = gtk_widget_real_remove_accelerator;
   klass->grab_focus = gtk_widget_real_grab_focus;
   klass->event = NULL;
   klass->button_press_event = NULL;
@@ -4907,4 +4918,30 @@ gtk_widget_class_path (GtkWidget *widget
       *path_p = g_strdup (rev_path);
       g_strreverse (*path_p);
     }
+}
+
+static gint
+gtk_widget_real_add_accelerator (GtkWidget *widget, guint accel_signal_id,
+				 GtkAccelGroup *accel_group, guint accel_key,
+				 GdkModifierType accel_mods, GtkAccelFlags accel_flags)
===========

I'll let Tim comment on this one, but the thing is,
gtk_accel_group_handle_add() was meant to be used as the default
handler for remove_accelerator, so having to do a workaround like this
really cannot be the right way to do things.

[ Don't ask me to defend the virtualization of ::add_accelerator in
  this fashion. ]

Regards,
                                        Owen