[Spice-devel] [PATCH v2 3/3] style: prefer early return to large if blocks

[Frediano Ziglio]

> 
> ---
>  server/red_worker.c | 46 ++++++++++++++++++++++++----------------------
>  1 file changed, 24 insertions(+), 22 deletions(-)
> 
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 9738e98..e23c460 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -991,11 +991,12 @@ static void release_image_item(ImageItem *item)
>  
>  static void release_upgrade_item(RedWorker* worker, UpgradeItem *item)
>  {
> -    if (!--item->refs) {
> -        red_worker_drawable_unref(worker, item->drawable);
> -        free(item->rects);
> -        free(item);
> -    }
> +    if (--item->refs)
> +        return;
> +
> +    red_worker_drawable_unref(worker, item->drawable);
> +    free(item->rects);
> +    free(item);
>  }
>  
>  static uint8_t *common_alloc_recv_buf(RedChannelClient *rcc, uint16_t type,
>  uint32_t size)
> @@ -1192,27 +1193,28 @@ static void red_worker_drawable_unref(RedWorker
> *worker, Drawable *drawable)
>  {
>      RingItem *item, *next;
>  
> -    if (!--drawable->refs) {
> -        spice_assert(!drawable->tree_item.shadow);
> -        spice_assert(ring_is_empty(&drawable->pipes));
> +    if (--drawable->refs)
> +        return;
>  
> -        if (drawable->stream) {
> -            red_detach_stream(worker, drawable->stream, TRUE);
> -        }
> -        region_destroy(&drawable->tree_item.base.rgn);
> +    spice_return_if_fail(!drawable->tree_item.shadow);
> +    spice_return_if_fail(ring_is_empty(&drawable->pipes));
>  

Here 2 spice_assert are converted to spice_return_if_fail.
They are just adding more leaks to happened leaks.
I would possibly suggest spice_warn_if or even better something like

   if (condition)
      spice_critical(message);

> -        remove_drawable_dependencies(worker, drawable);
> -        red_dec_surfaces_drawable_dependencies(worker, drawable);
> -        red_destroy_surface(worker, drawable->surface_id);
> +    if (drawable->stream) {
> +        red_detach_stream(worker, drawable->stream, TRUE);
> +    }
> +    region_destroy(&drawable->tree_item.base.rgn);
>  
> -        RING_FOREACH_SAFE(item, next, &drawable->glz_ring) {
> -            SPICE_CONTAINEROF(item, RedGlzDrawable, drawable_link)->drawable
> = NULL;
> -            ring_remove(item);
> -        }
> -        put_red_drawable(worker, drawable->red_drawable,
> drawable->group_id);
> -        free_drawable(worker, drawable);
> -        worker->drawable_count--;
> +    remove_drawable_dependencies(worker, drawable);
> +    red_dec_surfaces_drawable_dependencies(worker, drawable);
> +    red_destroy_surface(worker, drawable->surface_id);
> +
> +    RING_FOREACH_SAFE(item, next, &drawable->glz_ring) {
> +        SPICE_CONTAINEROF(item, RedGlzDrawable, drawable_link)->drawable =
> NULL;
> +        ring_remove(item);
>      }
> +    put_red_drawable(worker, drawable->red_drawable, drawable->group_id);
> +    free_drawable(worker, drawable);
> +    worker->drawable_count--;
>  }
>  
>  static inline void remove_shadow(RedWorker *worker, DrawItem *item)
> --
> 2.4.3
> 

Frediano