Re: master ecdbf82cb9: Fix leaks of XImage structures in image.c

[Stefan Kangas]

Po Lu via Mailing list for Emacs changes <emacs-diffs@gnu.org> writes:

> branch: master
> commit ecdbf82cb920f0648b6398502091868bb1bf7829
> Author: Po Lu <luangruo@yahoo.com>
> Commit: Po Lu <luangruo@yahoo.com>
>
>     Fix leaks of XImage structures in image.c
>
>     * src/image.c (image_clear_image, lookup_image): Fix coding
>     style.
>     (x_destroy_x_image): Remove unnecessary assertion.  Call
>     XDestroyImage, since otherwise only the image data is freed.
>     (image_from_emacs_colors): Rename variables to make more sense.
> ---
>  src/image.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/src/image.c b/src/image.c
> index 1e323ba66a..80b814cb1c 100644
> --- a/src/image.c
> +++ b/src/image.c
> @@ -1843,7 +1843,9 @@ image_clear_image (struct frame *f, struct image *img)
>  {
>    block_input ();
>    image_clear_image_1 (f, img,
> -                CLEAR_IMAGE_PIXMAP | CLEAR_IMAGE_MASK | CLEAR_IMAGE_COLORS);
> +                    (CLEAR_IMAGE_PIXMAP
> +                     | CLEAR_IMAGE_MASK
> +                     | CLEAR_IMAGE_COLORS));
>    unblock_input ();
>  }
>
> @@ -2980,7 +2982,8 @@ lookup_image (struct frame *f, Lisp_Object spec, int 
> face_id)
>        unblock_input ();
>      }
>
> -  /* We're using IMG, so set its timestamp to `now'.  */
> +  /* IMG is now being used, so set its timestamp to the current
> +     time.  */
>    img->timestamp = current_timespec ();
>
>    /* Value is the image id.  */
> @@ -3238,12 +3241,13 @@ x_create_x_image_and_pixmap (struct frame *f, int 
> width, int height, int depth,
>  static void
>  x_destroy_x_image (XImage *ximg)
>  {
> -  eassert (input_blocked_p ());
>    if (ximg)
>      {
>        xfree (ximg->data);
>        ximg->data = NULL;
>      }
> +
> +  XDestroyImage (ximg);
>  }
>
>  # if !defined USE_CAIRO && defined HAVE_XRENDER
> @@ -6224,26 +6228,28 @@ static void
>  image_from_emacs_colors (struct frame *f, struct image *img, Emacs_Color 
> *colors)
>  {
>    int x, y;
> -  Emacs_Pix_Container oimg = NULL;
> +  Emacs_Pix_Container ximage;
>    Emacs_Color *p;
>
> +  ximage = NULL;

Why not just keep the initialization where it was?  I thought we
preferred slowly moving to the C99 style over time.  This change goes in
the opposite direction.

> +
>    init_color_table ();
>
>    image_clear_image_1 (f, img, CLEAR_IMAGE_PIXMAP | CLEAR_IMAGE_COLORS);
>    image_create_x_image_and_pixmap (f, img, img->width, img->height, 0,
> -                                &oimg, 0);
> +                                &ximage, 0);
>    p = colors;
>    for (y = 0; y < img->height; ++y)
>      for (x = 0; x < img->width; ++x, ++p)
>        {
>       unsigned long pixel;
>       pixel = lookup_rgb_color (f, p->red, p->green, p->blue);
> -     PUT_PIXEL (oimg, x, y, pixel);
> +     PUT_PIXEL (ximage, x, y, pixel);
>        }
>
>    xfree (colors);
>
> -  image_put_x_image (f, img, oimg, 0);
> +  image_put_x_image (f, img, ximage, false);
>  #ifdef COLOR_TABLE_SUPPORT
>    img->colors = colors_in_color_table (&img->ncolors);
>    free_color_table ();