Re: [evolution-patches] patch for #62030 (calendar)

[Jeffrey Stedfast <fejj ximian com>]

On Thu, 2004-08-05 at 11:16, Rodrigo Moya wrote:
> On Thu, 2004-08-05 at 16:31 +0200, Rodrigo Moya wrote:
> > On Thu, 2004-08-05 at 10:09 -0400, Jeffrey Stedfast wrote:
> > > eh? you don't need to g_free() alloca()'d memory.
> > > 
> > right, valgrind was bailing about this, and didn't see the g_alloca
> > call. So, yes, discard this patch.
> > 
> > The real fix for the bug I was trying to fix is attached.
> >
> ok, hopefully the last patch, now including a fix in e-icon-factory.c
> and some cleanup in the alarm daemon.

only commenting on the icon_factory bit

> Index: e-util/e-icon-factory.c
> ===================================================================
> RCS file: /cvs/gnome/evolution/e-util/e-icon-factory.c,v
> retrieving revision 1.9
> diff -u -p -r1.9 e-icon-factory.c
> --- e-util/e-icon-factory.c     3 Jun 2004 15:01:33 -0000       1.9
> +++ e-util/e-icon-factory.c     5 Aug 2004 15:09:05 -0000
> @@ -196,6 +196,7 @@ e_icon_factory_init (void)
>  static void
>  icon_foreach_free (gpointer key, gpointer value, gpointer user_data)
>  {
> +       g_free (key);
>         icon_free (value);
>  }

I'd prefer that we simply used icon->name as the key like we currently
do, then this wouldn't be necessary.

>  
> @@ -287,7 +288,7 @@ e_icon_factory_get_icon (const char *ico
>                         return gdk_pixbuf_scale_simple
> (broken16_pixbuf, size, size, GDK_INTERP_NEAREST);
>         }
>         
> -       icon_key = g_alloca (strlen (icon_name) + 7);
> +       icon_key = g_malloc (strlen (icon_name) + 7);
>         sprintf (icon_key, "%dx%d/%s", size, size, icon_name);

if we're going to malloc, might as well just use g_strdup_printf here.
however, I'm not sure we really want to malloc. I think the better
solution is to simply pass icon_key to load_icon() as well. Then
load_icon() can pass icon_key to icon_new() rather than icon_name like
it currently does (which is the bit that causes the leak).

if we do it this way, then very few lines of code need to change :)

Jeff

>         
>         pthread_mutex_lock (&lock);
> @@ -301,11 +302,12 @@ e_icon_factory_get_icon (const char *ico
>                            icon is requested.  */
>                         
>                         icon = icon_new (icon_key, NULL);
> -                       g_hash_table_insert (name_to_icon, icon->name,
> icon);
> +                       g_hash_table_insert (name_to_icon, icon_key,
> icon);
>                 } else {
> -                       g_hash_table_insert (name_to_icon, icon->name,
> icon);
> +                       g_hash_table_insert (name_to_icon, icon_key,
> icon);
>                 }
> -       }
> +       } else
> +               g_free (icon_key);
>         
>         if ((pixbuf = icon->pixbuf)) {
>                 g_object_ref (pixbuf);
> @@ -343,15 +345,13 @@ e_icon_factory_get_icon_list (const char
>         
>         pthread_mutex_lock (&lock);
>         
> -       icon_key = g_alloca (strlen (icon_name) + 9);
> -       
>         for (i = 0; i < G_N_ELEMENTS (icon_list_sizes); i++) {
>                 size = icon_list_sizes[i];
> -               sprintf (icon_key, "%dx%d/%s", size, size, icon_name);
> +               icon_key = g_strdup_printf ("%dx%d/%s", size, size,
> icon_name);
>                 
>                 if (!(icon = g_hash_table_lookup (name_to_icon,
> icon_key))) {
>                         if ((icon = load_icon (icon_name, size,
> FALSE)))
> -                               g_hash_table_insert (name_to_icon,
> icon->name, icon);
> +                               g_hash_table_insert (name_to_icon,
> icon_key, icon);
>                 }

leaking icon_key here :)

>                 
>                 if (icon && icon->pixbuf) {