Re: gtk+ warning-reducing patch

[Darin Adler <darin bentspoon com>]

On Tuesday, July 17, 2001, at 06:50  PM, Owen Taylor wrote:

> > Index: configure.in
> > ===================================================================
> > RCS file: /cvs/gnome/gtk+/configure.in,v
> > retrieving revision 1.220
> > diff -p -u -r1.220 configure.in
> > --- configure.in	2001/06/28 17:12:31	1.220
> > +++ configure.in	2001/07/13 00:32:31
> > @@ -94,8 +94,6 @@ AM_PROG_LIBTOOL
> >   dnl Initialize maintainer mode
> >   AM_MAINTAINER_MODE
> >
> > -AC_CANONICAL_HOST
> > -
> >   dnl figure debugging default, prior to $ac_help setup
> >   dnl
> >   AC_DIVERT_PUSH(AC_DIVERSION_NOTICE)dnl
>
> Can you explain this change more? We do use the $host variables,
> though I can easily imagine that this does get called implicitely
> somewhere else.

As far as I can tell, the macros AC_LIBTOOL_SETUP, 
AC_LIBTOOL_SYS_GLOBAL_SYMBOL_PIPE, AC_PROG_LD, and AC_CHECK_LIBM all take 
care of invoking AC_CANONICAL_HOST. I think that invoking AM_PROG_LIBTOOL 
ends up having a side effect of taking care of AC_CANONICAL_HOST, and 
including it explicitly as well results in a warning about using it twice.

Perhaps removing it is not correct. I'm not enough of an automake/autoconf 
guru to be absolutely sure, and I'm beginning to have doubts after your 
(admittedly mild) challenge.

> > diff -p -u -r1.38 gdkrgb.c
> > --- gdk/gdkrgb.c	2001/03/02 17:06:06	1.38
> > +++ gdk/gdkrgb.c	2001/07/13 00:32:44
> > @@ -63,6 +63,7 @@
> >
> >   #include "gdk.h"		/* For gdk_flush() */
> >   #include "gdkrgb.h"
> > +#include "gdkimage.h"
> >
> >   typedef struct _GdkRgbInfo     GdkRgbInfo;
> >   typedef struct _GdkRgbCmapInfo GdkRgbCmapInfo;
>
> Does this really fix anything? (we are including gdk.h, after all)
> I think the warning that we were getting here is because
> gdk_image_new_bitmap() is #ifdef GDK_ENABLE_BROKEN.

I think you're right. Oops. Sorry.

> > diff -p -u -r1.30 gdkcolor-x11.c
> > --- gdk/x11/gdkcolor-x11.c	2001/02/13 05:44:46	1.30
> > +++ gdk/x11/gdkcolor-x11.c	2001/07/13 00:32:45
> > @@ -522,7 +522,7 @@ gdk_colors_free (GdkColormap *colormap,
> >
> >     if (npixels)
> >       XFreeColors (private->xdisplay, private->xcolormap,
> > -		 pixels, npixels, planes);
> > +		 (long *) pixels, npixels, planes);
> >     g_free (pixels);
> >   }
>
> I don't understand this cast:
>
> There seems to be a match between:
>
>   gulong *pixels;
>
> and:
>
>       XFreeColors(display, colormap, pixels, npixels, planes)
>              Display *display;
>              Colormap colormap;
>              unsigned long pixels[];
>              int npixels;
>              unsigned long planes;

I'm not sure how I got confused about this one and did it wrong. Let me 
look into it further.

> > @@ -610,7 +610,7 @@ gdk_colormap_alloc1 (GdkColormap *colorm
> >   	  if (private->info[ret->pixel].ref_count) /* got a duplicate */
> >   	    {
> >   	      XFreeColors (private->xdisplay, private->xcolormap,
> > -			   &ret->pixel, 1, 0);
> > +			   (long *) &ret->pixel, 1, 0);
> >   	    }
> >   	  else
> >   	    {
>
> This isn't right, is it GdkColor.pixel is a guint32 not a long.

I guess I'll need a temporary gulong to pass to XFreeColors? Sorry, I seem 
to have mucked this one up.

> > Index: gdk/x11/gdkwindow-x11.c
> > ===================================================================
> > RCS file: /cvs/gnome/gtk+/gdk/x11/gdkwindow-x11.c,v
> > retrieving revision 1.122
> > diff -p -u -r1.122 gdkwindow-x11.c
> > --- gdk/x11/gdkwindow-x11.c	2001/07/06 19:40:59	1.122
> > +++ gdk/x11/gdkwindow-x11.c	2001/07/13 00:32:50
> > @@ -3601,6 +3601,8 @@ update_pos (gint new_root_x,
> >             w += dx;
> >             h += dy;
> >             break;
> > +        default:
> > +          break;
> >           }
>
> Could you leave this one out. This code needs to be completed,
> and I'd rather have a warning than nothing.

Of course -- I'll be happy to leave out that change. I didn't realize 
there was missing code; I thought the other cases were "can never happen" 
type things.

I'll check in the ones I'm sure about shortly, and leave out all of these 
questionable ones for now.

    -- Darin